refactor: load eq_param_index of BundleSolvers correctly#212
refactor: load eq_param_index of BundleSolvers correctly#212sathvikbhagavan wants to merge 2 commits intomasterfrom
Conversation
|
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
| 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.
| 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.
3 participants
fixes: #211