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

[wpimath] Add cosineScale method to SwerveModuleState and instance optimize #7114

Merged
merged 16 commits into from
Sep 30, 2024

Conversation

narmstro2020
Copy link
Contributor

This PR adds an instance method for cosineScaling to the SwerveModuleState object and makes the static optimize method an instance method.

@narmstro2020 narmstro2020 requested a review from a team as a code owner September 21, 2024 04:05
Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cosineScale() needs a unit test, and a C++ port is needed. Otherwise, looks good.

@narmstro2020
Copy link
Contributor Author

cosineScale() needs a unit test, and a C++ port is needed. Otherwise, looks good.

No problem. Working on it.

Also I should've mentioned earlier. This makes cosineScaling more visible. I just stumbled on this last year when doing a random look through examples.

@narmstro2020
Copy link
Contributor Author

cosineScale() needs a unit test, and a C++ port is needed. Otherwise, looks good.

Should I model the unit test like the test for optimize?

@calcmogul
Copy link
Member

calcmogul commented Sep 21, 2024

Should I model the unit test like the test for optimize?

You could just write a single test that tests a positive and negative speed for different angles around the unit circle (e.g., 0°, 45°, -45°, 135°, -135°, 180°).

@narmstro2020 narmstro2020 requested a review from a team as a code owner September 21, 2024 04:15
@narmstro2020
Copy link
Contributor Author

Should I model the unit test like the test for optimize?

You could just write a single test that tests a positive and negative speed for different angles around the unit circle (e.g., 0°, 45°, -45°, 135°, -135°, 180°).

Just to clarify. Just pick an angle. And test both positive and negative speed against it.

@calcmogul
Copy link
Member

calcmogul commented Sep 21, 2024

Yea, for a range of angles. Just needs to be a representative sample of inputs.

@narmstro2020
Copy link
Contributor Author

What are my c++ issues in the example SwerveModule's?

@Gold856
Copy link
Contributor

Gold856 commented Sep 21, 2024

You need to switch all uses of the static method to the instance method, including in the C++ tests.

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Sep 21, 2024

You need to switch all uses of the static method to the instance method, including in the C++ tests.

I was getting local build errors on just using the void method. I wanted to clear that up before the test. Maybe I was misreading things.

@narmstro2020
Copy link
Contributor Author

/format

@PeterJohnson PeterJohnson merged commit fe80d72 into wpilibsuite:main Sep 30, 2024
34 checks passed
Gold856 pushed a commit to Gold856/allwpilib that referenced this pull request Oct 10, 2024
Gold856 pushed a commit to Gold856/allwpilib that referenced this pull request Oct 10, 2024
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.

4 participants