Skip to content
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

More flexible processing of ODEs #19

Merged
merged 217 commits into from
May 14, 2020

Conversation

clinssen
Copy link
Contributor

@clinssen clinssen commented Nov 7, 2019

Notes for reviewers:

Potential improvements (all of these are mentioned in the documentation):

  • Singularities in propagators
  • Poor sympy performance when simplifying large expressions
  • No support for analytically solvable inhomogeneous ODEs

C.A.P. Linssen added 30 commits April 9, 2019 20:22
@maedoc
Copy link
Collaborator

maedoc commented Apr 7, 2020

I read through the docs, code and tests, and I set up an environment, ran the test suite, python setup.py installed and ran a few examples. It all looks quite nice, straigthforward to use and well implemented. I have few comments:

You should have at least one sentence docstring for each element of your API, currently several are empty. If the API element is not for public consumption, it should be prefixed with a _ or perhaps hidden from the API docs (Sphinx has a decorator I think). Sometimes this requires a bit of imagination, e.g. get_jacobian is obvious what it does, but instaed of a docstraing like "Returns the Jacobian matrix" you can mention e.g. the convention of derivatives defined across rows versus columns.

Python's coding conventions are ignored in several places, and while I think that can become a yak-shaving thing, I would suggest applying autopep8 or black once just to get it all into shape, and then forget about it. There are a few functions that are a bit long for my taste (30 to 50 lines, nested control flow), but you should take into account your fellow developers and their taste as well.

Finally, I want to point out that Scipy's odeint does automatic switching between stiff and non-stiff methods and can return method choice and step size information for every step,

sol, info = scipy.integrate.odeint(f, y0, t, (), jac, full_output=True)
step_size_per_step = info['hu']
method_used_per_step = info['mused']
stiff_steps = (method_used_per_step == 2).sum()
non_stiff_steps = sol.size - stiff_steps
# etc

given your API, it would be straightforward to provide this as a fallback when PyGSL is not available (and looking at PyGSL's website does not inspire confidence as a long-term dependency).

That's all, for now; if you want me to look at other things, let me know.

@clinssen
Copy link
Contributor Author

clinssen commented May 9, 2020

Thank you for the feedback and your kind words! We went through your review and have addressed the points that you raised in the following way:

  • The coverage of the API documentation has been considerably extended.
  • The code has been cleaned up following Python syntax standards (e.g. underscore prefix indicating "private" members).
  • We absolutely agree that an alternative to (Py)GSL should be provided. We interpret this suggestion as a feature request/enhancement and will set this as a milestone for the subsequent release of ODE-toolbox. An issue has been created (Add Scipy ODE integrator as fallback for PyGSL #20).

As no radical changes were made to ODE-toolbox, we feel it would not be necessary to perform a formal second round of reviews. We are very grateful for your comments and will continue our improvements to ODE-toolbox on their basis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants