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

Conversation

@Dpananos
Copy link
Contributor

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

This comment has been minimized.

Copy link

review-notebook-app bot commented Aug 15, 2019

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

This comment has been minimized.

Copy link
Contributor

michaelosthege commented Aug 16, 2019

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

@Dpananos

This comment has been minimized.

Copy link
Contributor Author

Dpananos commented Aug 16, 2019

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

@ColCarroll

This comment has been minimized.

Copy link
Member

ColCarroll commented Aug 16, 2019

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 left a comment

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

This comment has been minimized.

Copy link
Contributor Author

Dpananos commented Aug 17, 2019

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

This comment has been minimized.

Copy link
Contributor

michaelosthege commented Aug 23, 2019

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

This comment has been minimized.

Copy link
Member

twiecki commented Aug 24, 2019

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

@Dpananos

This comment has been minimized.

Copy link
Contributor Author

Dpananos commented Aug 24, 2019

@twiecki Ran black on all the new files.

Copy link
Contributor

michaelosthege left a comment

@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.

pymc3/ode/ode.py Show resolved Hide resolved
@twiecki

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

Dpananos commented Aug 25, 2019

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

This comment has been minimized.

Copy link
Contributor

canyon289 commented Aug 25, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

Dpananos commented Aug 27, 2019

@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
Dpananos and others added 4 commits Aug 28, 2019
Co-Authored-By: Thomas Wiecki <thomas.wiecki@gmail.com>
Co-Authored-By: Thomas Wiecki <thomas.wiecki@gmail.com>
a compiled function which allows for computation of gradients of the
differential equation's solition with repsect to the parameters.
Args:

This comment has been minimized.

Copy link
@twiecki

twiecki Sep 3, 2019

Member

These still have the old doc-format.

This comment has been minimized.

Copy link
@twiecki

twiecki Sep 3, 2019

Member

Merged and fixed on master.

@twiecki twiecki merged commit c7a41c3 into pymc-devs:master Sep 3, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 89.558%
Details
@twiecki

This comment has been minimized.

Copy link
Member

twiecki commented Sep 3, 2019

Congrats again @Dpananos, this was a beast!

@Dpananos Dpananos deleted the Dpananos:gsoc_ode branch Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.