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 simulated annealing #5961

Merged

Conversation

calcmogul
Copy link
Member

Co-authored-by: Ashray._.g ashray.gupta@gmail.com

@calcmogul calcmogul requested review from PeterJohnson and a team as code owners November 25, 2023 02:19
@calcmogul calcmogul force-pushed the wpimath-add-simulated-annealing branch 2 times, most recently from 6e6a936 to 79f88d6 Compare November 25, 2023 02:25
@calcmogul calcmogul added the breaking Introduces a breaking change. label Nov 25, 2023
@calcmogul
Copy link
Member Author

calcmogul commented Nov 25, 2023

The circular buffer changes are there to make the tests easier to write, and we've wanted a more generic circular buffer class for a while. That was a breaking change though to make it match how we organized the interpolating tree maps.

The traveling salesman tests take 15 ms on my machine, which is rather slow given the size of the problem. I haven't looked into better convergence criteria. Part of the issue is you need to let the randomness stew a bit so the solver has a chance to reach a more optimal solution.

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Would it be worth splitting the CircularBuffer changes into a different PR (e.g. for commit history)?

@calcmogul
Copy link
Member Author

Depends on #5969.

@calcmogul calcmogul removed the breaking Introduces a breaking change. label Nov 27, 2023
@calcmogul calcmogul requested a review from a team as a code owner November 27, 2023 20:16
@calcmogul calcmogul force-pushed the wpimath-add-simulated-annealing branch from 340d013 to 90e79b8 Compare November 27, 2023 20:21
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Looks good to me (for what it's worth), just a few things that escaped the renaming.

@calcmogul calcmogul force-pushed the wpimath-add-simulated-annealing branch 9 times, most recently from 0f91d7f to f94da27 Compare November 28, 2023 00:58
@calcmogul calcmogul marked this pull request as draft December 1, 2023 04:12
Co-authored-by: Ashray._.g <ashray.gupta@gmail.com>
@calcmogul calcmogul force-pushed the wpimath-add-simulated-annealing branch from f94da27 to d5731a0 Compare December 1, 2023 05:14
@calcmogul calcmogul marked this pull request as ready for review December 1, 2023 05:14
@PeterJohnson PeterJohnson merged commit ac7d726 into wpilibsuite:main Dec 1, 2023
25 of 26 checks passed
@calcmogul calcmogul deleted the wpimath-add-simulated-annealing branch December 1, 2023 06:59
Starlight220 pushed a commit to Starlight220/allwpilib that referenced this pull request Dec 1, 2023
Co-authored-by: Ashray._.g <ashray.gupta@gmail.com>
This pull request was closed.
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