-
Notifications
You must be signed in to change notification settings - Fork 55
[RTM] multi-start values testing #570
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
Conversation
pariterre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 7 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EveCharbie)
bioptim/examples/getting_started/custom_initial_guess.py line 20 at r3 (raw file):
import numpy as np import biorbd_casadi as biorbd from bioptim.misc.enums import MagnitudeType
This seems related to @rapidnico PR. But it should not appear here. I actually specifically asked in the other PR to change this... If your PR are related, merge your work in a single PR, otherwise I am bound to repeat myself not knowing who is responsible for the changes
bioptim/limits/path_conditions.py line 1150 at r3 (raw file):
raise ValueError("bounds must be specified to generate noised initial guess") if len(bounds) != nb_phases:
I assume all these changes are not yours?
Code quote:
@staticmethod
def _check_type_and_format_bounds(bounds, nb_phases):
"""
Check bounds type and format
Parameters
----------
nb_phases
The number of phases
"""
if bounds is None:
raise ValueError("bounds must be specified to generate noised initial guess")
if len(bounds) != nb_phases:
raise ValueError(f"Invalid size of 'bounds', 'bounds' must be size {nb_phases}")
return boundstests/test_update_bounds_and_init.py line 5 at r3 (raw file):
import biorbd_casadi as biorbd from casadi import MX from bioptim.misc.enums import MagnitudeType
This should not appear as is, as specified in the @rapidnico PR
EveCharbie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR was not RTR. Yes I merged Rapidnico's branch into mine in the mean time. But I will surely wait until his PR is accepted before asking you to review ;)
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EveCharbie)
pariterre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)
Codecov ReportBase: 80.53% // Head: 80.53% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #570 +/- ##
=======================================
Coverage 80.53% 80.53%
=======================================
Files 95 95
Lines 10498 10498
=======================================
Hits 8455 8455
Misses 2043 2043
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
pariterre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)
|
@EveCharbie RTM? |
|
@pariterre yep :) |
All Submissions:
New Feature Submissions:
black . -l120 --exclude "external/*")?Changes to Core Features:
This change is