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

Add Differential Equation API #3590

Merged
merged 21 commits into from
Sep 3, 2019
Merged

Add Differential Equation API #3590

merged 21 commits into from
Sep 3, 2019

Conversation

Dpananos
Copy link
Contributor

@Dpananos Dpananos commented Aug 15, 2019

This work is a part of Google Summer of Code 2019.

This PR adds:

  • ode submodule, which contains DifferentialEquation, the main tool for doing inference with ODEs.

  • An ODEGradop for computation of the gradients required for HMC.

  • A function for computing an augmented system of ODEs (that is, the original ODE + ODEs for the sensitivity equations) called augment_system.

  • Unit tests for ode

  • A Notebook showcasing how to use DifferentialEquation in a couple of examples regarding a free falling object and a spread of an infection.

  • Rerun existing ODE notebook for comparison


Sorry, I was having some issues with the last PR so I thought I would just start again.Here is a link to the old PR should you need to reference it.

@review-notebook-app
Copy link

Check out this pull request on ReviewNB: https://app.reviewnb.com/pymc-devs/pymc3/pull/3590

You'll be able to see notebook diffs and discuss changes. Powered by ReviewNB.

@michaelosthege
Copy link
Member

Could the test failure be related to this issue? #1640

@Dpananos
Copy link
Contributor Author

FYI: Test pass now because I have included the pytest.xfail decorator on previously failing tests. See pymc3/tests/test_ode.py

@ColCarroll
Copy link
Member

I think that's a good choice. For reference, this means it might not work on GPUs -- I am not sure if that is an OK tradeoff for ODEs, but I do think such a failure should not block this PR. We should remember to open an issue once it is merged, though.

Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few nitpicky things, and one musing suggestion. None of them are dealbreakers, and I will be delighted when this gets merged, even if none of the changes get made.

As another musing suggestion, running

python -m pylint --enable=all pymc3/step_methods/hmc/nuts.py

on each file you changed -- many suggestions will be silly, or ones we do not enforce, but some are helpful.

If you want to be really fancy

git diff --name-only HEAD master | xargs python -m pylint --enable=all

will pipe just the changed files through pylint

pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/utils.py Outdated Show resolved Hide resolved
pymc3/tests/test_ode.py Outdated Show resolved Hide resolved
pymc3/tests/test_ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
@Dpananos
Copy link
Contributor Author

Thanks @ColCarroll, I think I added everything you suggested.

pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Show resolved Hide resolved
@Dpananos Dpananos changed the title WIP: Add Differential Equation API Add Differential Equation API Aug 19, 2019
@michaelosthege
Copy link
Member

Your module is currently not imported by default. Please add this after line 10 of pymc3\__init__.py:

from . import ode

(just like gp is imported in line 7)

pymc3/ode/__init__.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
@twiecki
Copy link
Member

twiecki commented Aug 24, 2019

As this is new code, can you just run black on the new files?

@Dpananos
Copy link
Contributor Author

@twiecki Ran black on all the new files.

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dpananos running black gave some nice results there! Can you also run it on test_ode.py?

@twiecki @ColCarroll Are we ready to merge?

There are some improvements regarding the returned shape & the grad that will follow in the near future, but the functionality is already there with this PR.

@twiecki
Copy link
Member

twiecki commented Aug 25, 2019

It seems like black was run on just ode.py, can you run it on all new files here?

@Dpananos
Copy link
Contributor Author

I ran black on the pymc3/ode directory. It left the majority of the files unchanged. I made the change in the docstring, and also ran black on the test_ode.py file.

pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
@canyon289
Copy link
Member

Just some minor comments on some code style. The notebook could also use spaces on the inline comments. I know its picky, but for me its jarring after seeing a bunch of PEP8 style comments.

On the notebook the PyMC3 version is shown, just to double check will that be the version of PyMC3 that includes the ODE solver, or will we increment the PyMC3 version to 3.8 once this ODE solver get merged to master?

@twiecki
Copy link
Member

twiecki commented Aug 26, 2019

Need to add to release-notes and link the new NBs to the docs.

RELEASE-NOTES.md Outdated Show resolved Hide resolved
@Dpananos
Copy link
Contributor Author

@twiecki Not sure how to update the docs, but I did find a reference to the other ODE notebook in docs/source/notebooks/table_of_contents_examples.js. I just swapped out my notebook for that one.

@canyon289 Changes made and made the notebook more pep-8 friendly wrt spaces (I will never get used to one space after assignment, no space after arguments).

If that's all, are we ready to merge?

RELEASE-NOTES.md Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
pymc3/ode/ode.py Outdated Show resolved Hide resolved
a compiled function which allows for computation of gradients of the
differential equation's solition with repsect to the parameters.

Args:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These still have the old doc-format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged and fixed on master.

@twiecki twiecki merged commit c7a41c3 into pymc-devs:master Sep 3, 2019
@twiecki
Copy link
Member

twiecki commented Sep 3, 2019

Congrats again @Dpananos, this was a beast!

@Dpananos Dpananos deleted the gsoc_ode branch September 5, 2019 23:14
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.

7 participants