-
Notifications
You must be signed in to change notification settings - Fork 46
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
[RTM] random_initial_condition_example #486
Conversation
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.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @EveCharbie and @Ipuch)
bioptim/examples/getting_started/custom_initial_guess.py
line 60 at r1 (raw file):
return my_values[:, 0] + (my_values[:, -1] - my_values[:, 0]) * current_shooting_point / n_shooting def generate_random_initial_guess(bounds: Bounds,
I would create this as a method of InitialGuess and add an extra argument of a new class called Noise.
it would look like this:
n = Noise(bounds, scaling, n_element: int)
x = InitialGuess(x, interpolation=initial_guess, noise=n)
bioptim/examples/getting_started/custom_initial_guess.py
line 75 at r1 (raw file):
scaling: int or list The amplitude of the gaussian noise generated n_elements: int
I'm not sure what it is.
bioptim/examples/getting_started/custom_initial_guess.py
line 99 at r1 (raw file):
return bounds_min_matrix, bounds_max_matrix bound_push = 0.1
This could be a nice option too :)
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.
Seems very good. I've left some minor changes to make and it should be ready to merge :)
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @EveCharbie and @Ipuch)
bioptim/examples/getting_started/custom_initial_guess.py
line 60 at r1 (raw file):
Previously, Ipuch (Pierre Puchaud) wrote…
I would create this as a method of InitialGuess and add an extra argument of a new class called Noise.
it would look like this:
n = Noise(bounds, scaling, n_element: int) x = InitialGuess(x, interpolation=initial_guess, noise=n)
makes sense, NoiseInitialGuess works for me too
bioptim/examples/getting_started/custom_initial_guess.py
line 67 at r2 (raw file):
n_shooting: int, final_time: float, automatic_random_init: bool = False,
I know this is in an example, so not so much important... I feel that automatic
in the name is a bit overkill... if you ask for random_init
it is kind of implied that you won't have to provided the noise, therefore it is automatic... Maybe just random_init
would be less confusing?
bioptim/examples/getting_started/custom_initial_guess.py
line 155 at r2 (raw file):
bounds=x_bounds, bound_t=None, scaling=1,
is scaling
the magnitude of the noise? If so, could we find a better term?
bioptim/limits/path_conditions.py
line 873 at r2 (raw file):
self.n_elements = n_elements self.type = init_interpolation
Could you double check that you actually have to override this many properties (it may be). I was just feeling for instance that bound
should be set from the super().__ini__(bound=bound, **parameters)
? Maybe there is no bound
parameters and I am just confused!
bioptim/limits/path_conditions.py
line 898 at r2 (raw file):
self.create_noise_matrix() self.init = InitialGuess(self.noised_initial_guess, interpolation=InterpolationType.EACH_FRAME).init
Doesn't super().__init__
already take care of this?
bioptim/limits/path_conditions.py
line 902 at r2 (raw file):
self.type = InterpolationType.EACH_FRAME def create_noise_matrix(self):
This should probably be a protected method (_create_noise_matrix
)
tests/test_global_getting_started.py
line 332 at r2 (raw file):
@pytest.mark.parametrize("interpolation", InterpolationType) @pytest.mark.parametrize("ode_solver", [OdeSolver.RK4, OdeSolver.RK8, OdeSolver.IRK]) def test_initial_guesses(automatic_random_init, interpolation, ode_solver):
if you decide to remove the automatic
you will have to do it here too
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.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @EveCharbie, @Ipuch, and @pariterre)
bioptim/examples/getting_started/custom_initial_guess.py
line 155 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
is
scaling
the magnitude of the noise? If so, could we find a better term?
Done.
bioptim/limits/path_conditions.py
line 873 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
Could you double check that you actually have to override this many properties (it may be). I was just feeling for instance that
bound
should be set from thesuper().__ini__(bound=bound, **parameters)
? Maybe there is nobound
parameters and I am just confused!
Need to talk about this. Sorry for my limited programming skills, but I thought this refers to the init of InitialGuess which only takes intial_guess and interpolation as parameters.
bioptim/limits/path_conditions.py
line 898 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
Doesn't
super().__init__
already take care of this?
I don't think so, because I overwrite the type since I Interpolate myself.
bioptim/limits/path_conditions.py
line 902 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
This should probably be a protected method (
_create_noise_matrix
)
Done.
tests/test_global_getting_started.py
line 332 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
if you decide to remove the
automatic
you will have to do it here too
Done.
provided initial guess to super class
test the random noise
fixed test initial guess
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
==========================================
+ Coverage 80.25% 80.33% +0.08%
==========================================
Files 85 86 +1
Lines 9268 9368 +100
==========================================
+ Hits 7438 7526 +88
- Misses 1830 1842 +12
Continue to review full report at Codecov.
|
docstring minor
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 2 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @EveCharbie, @Ipuch, and @pariterre)
bioptim/limits/path_conditions.py
line 873 at r2 (raw file):
Previously, EveCharbie (Eve Charbonneau) wrote…
Need to talk about this. Sorry for my limited programming skills, but I thought this refers to the init of InitialGuess which only takes intial_guess and interpolation as parameters.
You are probably right, I am free to discuss anyway :)
bioptim/limits/path_conditions.py
line 898 at r2 (raw file):
Previously, EveCharbie (Eve Charbonneau) wrote…
I don't think so, because I overwrite the type since I Interpolate myself.
I think I understand what you are doing, let's discuss by voice just to make sure
bioptim/limits/path_conditions.py
line 829 at r7 (raw file):
Methods ------- -create_noise_matrix(self)
I think you meant '_' instead of '-'?
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.
@Ipuch will add some tests and then we are done
Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EveCharbie and @Ipuch)
@pariterre RTM ? |
Yes :) |
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 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EveCharbie and @Ipuch)
@Ipuch and @pariterre
This is what I had in mind for the random initial guess.
For the cuteness and where to put it, I'll let you do it or tell me exactly what you want.
This change is