refactor: load eq_param_index of BundleSolvers correctly #212
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #212 +/- ##
=======================================
Coverage 84.46% 84.46%
=======================================
Files 21 21
Lines 3695 3695
=======================================
Hits 3121 3121
Misses 574 574 ☔ View full report in Codecov by Sentry. |
| @@ -1357,7 +1357,7 @@ def __init__(self, ode_system, conditions, t_min, t_max, | |||
|
|
|||
| def _diff_eqs_wrapper(*variables): | |||
| funcs_and_coords = variables[:N_FUNCTIONS + N_COORDS] | |||
| eq_params = tuple(variables[idx] for idx in eq_param_index) | |||
| eq_params = tuple(variables[idx] for idx in self.eq_param_index) | |||
There was a problem hiding this comment.
Is there a particular reason we want to use self.eq_param_index instead of eq_param_index here? Both ways should have the same effect most of the time, but I don't want self.diff_eqs to be a closure that depends on self. This would make certain operations complicated, such as recreating a solver
solver1 = BundleSolver1D(..., eq_param_index1)
solver2 = BundleSolver1D(..., eq_param_index2)
solver2.diff_eqs = solver1.diff_eqsIf eq_param_index1 and eq_param_index2 are different, the above won't work, as solver1.diff_eqs will always depend on solver1.
There was a problem hiding this comment.
hmm.. my motivation to use self.eq_param_index was to use this internal variable when loading a saved solver as otherwise we have to recompute eq_param_index again and pass it in the constructor.
| @@ -530,7 +530,9 @@ def load(cls, | |||
| t_min=t_min, | |||
| t_max=t_max, | |||
| theta_min=tuple(load_dict['solver'].r_min[1:]), | |||
| theta_max=tuple(load_dict['solver'].r_max[1:])) | |||
| theta_max=tuple(load_dict['solver'].r_max[1:]), | |||
| eq_param_index=load_dict['solver'].eq_param_index | |||
There was a problem hiding this comment.
At a certain point we should consider refactoring the solver_utils package. A lot of the code seems to be a little ad-hoc. They are fine for now.
There was a problem hiding this comment.
Yes, I think we are due to think about refactoring and redesigning these pieces as they are pretty hard coded. I also have some thoughts on how parameters are saved - https://github.com/NeuroDiffGym/neurodiffeq/blob/master/neurodiffeq/solvers_utils.py#L99 which ties back to how parameters are used inside the function of the differential equation. I will open up an issue on this and we can discuss these things in detail.
fixes: #211