Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sysid] Fix adjusted R² calculation #6101

Merged

Conversation

calcmogul
Copy link
Member

It hardcoded p to 2.

@calcmogul calcmogul requested a review from a team as a code owner December 27, 2023 02:09
@calcmogul calcmogul force-pushed the sysid-fix-adjusted-r2-calculation branch 2 times, most recently from 837f28a to 68356c4 Compare December 27, 2023 02:12
@calcmogul calcmogul force-pushed the sysid-fix-adjusted-r2-calculation branch from 68356c4 to dfb348a Compare December 27, 2023 02:28
@calcmogul
Copy link
Member Author

The C++ compiler accepts Greek letters but not superscripts.

@ncorrea210
Copy link
Contributor

Which one? I tried it using the utf8 character U+00B2 found here: https://www.compart.com/en/unicode/U+00B2 and it compiled fine with g++. (I deleted the comment just before that because it seemed a little nit-picky and unnecessary, I was just wondering).

@calcmogul
Copy link
Member Author

calcmogul commented Dec 27, 2023

g++ accepts those as extensions to the standard-mandated character set, but MSVC does not. Clang will accept them with a flag.

@ncorrea210
Copy link
Contributor

ncorrea210 commented Dec 27, 2023

Ah, I'm not familiar with MSVC or clang so didn't know. Otherwise everything LGTM here.

@calcmogul
Copy link
Member Author

calcmogul commented Dec 27, 2023

Was going off of memory doing work for Sleipnir. For the compilers FRC supports, looks like only the FRC 2023 compiler doesn't like superscripts: https://godbolt.org/z/5x74qKKEf. Seems to be conformance-related, because later versions of x86 GCC also fail if -pedantic is passed.

@PeterJohnson PeterJohnson merged commit f2c2bab into wpilibsuite:main Dec 27, 2023
27 checks passed
@calcmogul calcmogul deleted the sysid-fix-adjusted-r2-calculation branch December 27, 2023 04:41
r4stered pushed a commit to r4stered/allwpilib that referenced this pull request Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants