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

correctly parse optimizer params for base optimizer in gnc #1085

Merged
merged 2 commits into from
Feb 19, 2022
Merged

correctly parse optimizer params for base optimizer in gnc #1085

merged 2 commits into from
Feb 19, 2022

Conversation

yunzc
Copy link
Contributor

@yunzc yunzc commented Feb 2, 2022

Parse the parameters for the base optimizer in GNC.

@varunagrawal
Copy link
Collaborator

Awesome! I ran the CI and will merge once it passes. @yunzc you should probably also add a unit test to expose this "bug" so that it is caught in the future.

@yunzc
Copy link
Contributor Author

yunzc commented Feb 4, 2022

Sounds good @varunagrawal. I can't seem to find unit tests for Gnc in general, so it might be good to add that. I can add that to my TODO list.

@varunagrawal
Copy link
Collaborator

@yunzc can we expect that soon? A lack of unit tests really hurts confidence in our code.

@dellaert
Copy link
Member

So do we merge this or wait for a unit test? @varunagrawal don't merge without asking anyone :-/

@lucacarlone
Copy link
Contributor

I'll take a stab at adding a unit test over the weekend! ok @dellaert , @varunagrawal and @yunzc ?

@yunzc
Copy link
Contributor Author

yunzc commented Feb 17, 2022

@lucacarlone sounds good! Thanks!

@lucacarlone
Copy link
Contributor

@yunzc - pleasure!

@varunagrawal
Copy link
Collaborator

Yes, that's why I haven't merged it yet :)

@lucacarlone
Copy link
Contributor

@varunagrawal @yunzc : done! this is ready to go after the CI finishes.

@varunagrawal
Copy link
Collaborator

We should be good to merge this now

@dellaert dellaert merged commit 686e16a into borglab:develop Feb 19, 2022
@dellaert
Copy link
Member

Done!

@lucacarlone
Copy link
Contributor

yay!

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