Timesteps bugs

Zebedee Nichollsrequested to merge
timesteps-clarity into main

Description

Following on from !34 (merged), this attempts to handle a few edge cases that aren't handled in !34 (merged) (the new tests demonstrate the cases). The other thing I found really confusing in !34 (merged) was the flow and which parameters were needed for which branches. So, as part of handling the edge cases, I've also cleaned up the flow and made which bits of information are required for which branch clearer.

If I'm completely honest, I'm not sure this is that much clearer than what is in https://gitlab.com/magicc/fgen/-/commits/timesteps-zn. I guess handling these sorts of patterns is just difficult in general, and even more so in Fortran. That's probably a good lesson for us to realise because things likely won't become simpler from here. I imagine we both underestimated how much logic is required to handle t_eval well in all the edge cases (a bunch of new ones I realised while writing this, which is why the logic here is so much more complicated than in !34 (merged)).

Having seen the two implementations now, I'd probably go somewhere in the middle (and I'm happy to do that refactoring as part of that MR if you agree) i.e.:

  • leave the branching logic in solve_ivp
  • but make the data objects we have here a bit smarter i.e. give them methods that can handle the preparation and recording steps. That would allow all of the related logic to be split out. I think it makes sense to use objects so we can just call their methods, rather than using pure functions where we'll have to pass all the t_eval specific parameters up and down, which would result in solve_ivp having heaps of variables (too many in my opinion) floating around.
  • don't go the whole way with dependency injection/polymorphism because it adds so many layers of misdirection and they're not worth it when there are only two branches

Merge Request Steps

Please confirm that this pull request has done the following:

  • Checked that the added tests fail in !34 (merged)
  • !34 (merged)
  • Tests added
  • Documentation added (where applicable)
  • Changelog item added to changelog/.
Edited by Zebedee Nicholls

Merge request reports