New notebook on ODEs drawing from the previously scatter work and upd… #478

gbrunkhorst · 2022-12-16T17:58:49Z

…ating based on the current state of the pymc examples

{Insert Description}

Notebook follows style guide https://docs.pymc.io/en/latest/contributing/jupyter_style.html
PR description contains a link to the relevant issue: a tracker one for existing notebooks or a proposal one for new notebooks
Check the notebook is not excluded from any pre-commit check: https://github.com/pymc-devs/pymc-examples/blob/main/.pre-commit-config.yaml

Helpful links

https://github.com/pymc-devs/pymc-examples/blob/main/CONTRIBUTING.md

review-notebook-app · 2022-12-16T17:58:53Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

review-notebook-app · 2022-12-18T01:42:31Z

View / edit / reply to this conversation on ReviewNB

Armavica commented on 2022-12-18T01:42:31Z
----------------------------------------------------------------

concentrations -> what would you think of "densities"? "concentrations" is usually associated with chemical solutions, where prey/predator dynamics can also happen but it is rarer
unknown -> unknowns (?)
I don't think beta is typically qualified as "natural", because it is caused by predation, as contrast with gamma for example, which is an intrinsic cause (aging for example)
"how many caught prey create a new predator" is actually represented by the number beta/delta, not by delta alone

gbrunkhorst commented on 2022-12-27T18:30:51Z
----------------------------------------------------------------

All comments addressed and will be reflected in the next pull request. Bullet for delta revised to:

* $\delta$ is the growing rate of predator in the presence of prey.

Hopefully this is accurate.

Armavica commented on 2022-12-29T00:17:04Z
----------------------------------------------------------------

Thank you for the changes and yes, I think that this formulation is accurate enough.

review-notebook-app · 2022-12-18T01:42:32Z

View / edit / reply to this conversation on ReviewNB

Armavica commented on 2022-12-18T01:42:31Z
----------------------------------------------------------------

Maybe "population" instead of "concentration"? Is there a unit?

gbrunkhorst commented on 2022-12-27T18:35:01Z
----------------------------------------------------------------

Comment addressed (in the next pull request). Going back to the reference in the Stan example, the units are actually 1000 pelts.

review-notebook-app · 2022-12-18T01:42:32Z

View / edit / reply to this conversation on ReviewNB

Armavica commented on 2022-12-18T01:42:32Z
----------------------------------------------------------------

Scipy's odeint is on the path to deprecation, do you think that it would be doable to update the notebook using the new interface solve_ivp? They have a very similar API, so the transition should normally be painless.

gbrunkhorst commented on 2022-12-27T18:46:08Z
----------------------------------------------------------------

Per my previous comment, odeint is faster than solve_ivp for some reason. I looped through each of the available solve_ivp methods and all were slower than odeint , even LSDOA, which should be the same solver as used in odeint! I got 3.2 ms for odeint compared to 12.1 ms solve_ivp on my old windows machine. Since we have to solve the ODE many 1,000s of times, I'm suggesting that we leave this one as is for now.

Armavica commented on 2022-12-29T00:15:40Z
----------------------------------------------------------------

Ok, thank you for investigating this! Let's keep it like this then.

review-notebook-app · 2022-12-18T01:42:33Z

View / edit / reply to this conversation on ReviewNB

Armavica commented on 2022-12-18T01:42:33Z
----------------------------------------------------------------

"to the know" -> "to know"

gbrunkhorst commented on 2022-12-27T18:47:17Z
----------------------------------------------------------------

Revised for next pull request.

review-notebook-app · 2022-12-18T01:42:34Z

View / edit / reply to this conversation on ReviewNB

Armavica commented on 2022-12-18T01:42:34Z
----------------------------------------------------------------

Line #1.    # Variable list to give to the sample step parameter

gbrunkhorst commented on 2022-12-27T18:48:19Z
----------------------------------------------------------------

Revised for next pull request.

Armavica · 2022-12-18T01:44:39Z

Thank you for your work, I like this tutorial a lot and even though I don't have much to say about the core of the work, I noted a couple of typos.

gbrunkhorst · 2022-12-20T16:24:20Z

Thank you @Armavica for the helpful comments. I will address all of them in the next week or so. The only comment I might not follow: it would be straight forward to swap in solve_ivp for odeint, but I seem to recall from speed tests that solve_ivp was surprisingly slower than odeint, so I'll double-check the speed comparison before making the change. Make sense?

Armavica · 2022-12-20T17:17:04Z

Sure, thank you! I am surprised by solve_ivp being slower than odeint though, but would be interested to know if this is really the case. Perhaps they just use a different default solver, in which case it should hopefully be possible to select in solve_ivp the same solver as odeint is using (or maybe even a faster one), with the method= argument.

review-notebook-app · 2022-12-22T19:53:58Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-12-22T19:53:57Z
----------------------------------------------------------------

I am not sure what is going on here, but it is not recognized by sphinx, tags and authors are not parsed, and the title is also missing. Make sure the cell is markdown cell and the breaklines follow the same pattern as in https://docs.pymc.io/en/latest/contributing/jupyter_style.html#first-cell. If you run pre-commit and push the myst I'll be able to diagnose what is going on much better, here in reviewnb it is impossible to tell.

I would recommend having only 1 level 1 heading in the notebook, here in this cell and removing the one right below.

gbrunkhorst commented on 2022-12-27T23:18:18Z
----------------------------------------------------------------

OK - I tried again and pushed changes that include the myst.md file. Let me know if you can see the second push. The issue was, I think, that the first cell was raw rather than markdown. Let me know if this works! Thanks!

review-notebook-app · 2022-12-22T19:53:59Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-12-22T19:53:58Z
----------------------------------------------------------------

The block equation is not being rendered correctly in the preview: https://pymcio--478.org.readthedocs.build/projects/examples/en/478/ode_models/ODE_Lotka_Volterra_multiple_ways.html#lotka-volterra-predator-prey-model, maybe it is not in its own line?

gbrunkhorst commented on 2022-12-27T20:30:43Z
----------------------------------------------------------------

It was on it's own line, but I added two spaces to the previous line - I hope this takes care of it.

review-notebook-app · 2022-12-22T19:54:00Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-12-22T19:53:59Z
----------------------------------------------------------------

you will need to turn of formatting in this cell: https://docs.pymc.io/en/latest/contributing/jupyter_style.html#pre-commit-and-code-formatting

gbrunkhorst commented on 2022-12-27T20:31:50Z
----------------------------------------------------------------

formatting turned off in next pull request.

review-notebook-app · 2022-12-22T19:54:00Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-12-22T19:54:00Z
----------------------------------------------------------------

Line #6.        elif SMC==True:

this part seems to never be used

gbrunkhorst commented on 2022-12-27T20:38:38Z
----------------------------------------------------------------

Good catch, in fact none of the control flow logic is used in the final version of the notebook, only the else: revised globally.

review-notebook-app · 2022-12-22T19:54:01Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-12-22T19:54:01Z
----------------------------------------------------------------

I think if you add labeller=az.label.NoVarLabeller() to plot_forest the plots will look nicer as it will avoid repeating the var name in every yticklabel (and it already is on the title)

gbrunkhorst commented on 2022-12-27T22:57:33Z
----------------------------------------------------------------

Agree it is busy to repeat. az.labels.NoVarLabeller() gave busy "None" text, so I cleaned it up with a few lines of matplotlib code to address the comment.

gbrunkhorst commented on 2023-01-12T17:30:08Z
----------------------------------------------------------------

Note, we both seem to be on Arviz 0.14 - I'm not sure why az.labels.NoVarLabeller() is acting differently but suggest we forge ahead with the work around.

gbrunkhorst · 2022-12-23T18:00:08Z

thanks @OriolAbril I'll make changes in the next week or so.

gbrunkhorst · 2022-12-27T18:30:52Z

All comments addressed and will be reflected in the next pull request. Bullet for delta revised to:

* $\delta$ is the growing rate of predator in the presence of prey.

Hopefully this is accurate.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T18:35:02Z

Comment addressed (in the next pull request). Going back to the reference in the Stan example, the units are actually 1000 pelts.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T18:46:09Z

Per my previous comment, odeint is faster than solve_ivp for some reason. I looped through each of the available solve_ivp methods and all were slower than odeint , even LSDOA, which should be the same solver as used in odeint! I got 3.2 ms for odeint compared to 12.1 ms solve_ivp on my old windows machine. Since we have to solve the ODE many 1,000s of times, I'm suggesting that we leave this one as is for now.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T18:47:18Z

Revised for next pull request.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T18:48:20Z

Revised for next pull request.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T20:30:44Z

It was on it's own line, but I added two spaces to the previous line - I hope this takes care of it.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T20:31:51Z

formatting turned off in next pull request.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T20:38:39Z

Good catch, in fact none of the control flow logic is used in the final version of the notebook, only the else: revised globally.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T22:57:34Z

Agree it is busy to repeat. az.labels.NoVarLabeller() gave busy "None" text, so I cleaned it up with a few lines of matplotlib code to address the comment.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-27T23:18:19Z

OK - I tried again and pushed changes that include the myst.md file. Let me know if you can see the second push. The issue was, I think, that the first cell was raw rather than markdown. Let me know if this works! Thanks!

View entire conversation on ReviewNB

review-notebook-app · 2022-12-28T13:58:03Z

View / edit / reply to this conversation on ReviewNB

drbenvincent commented on 2022-12-28T13:58:02Z
----------------------------------------------------------------

The :author: component should only include the authors. The adaptation information goes just in the author block at the end of the notebook, see https://docs.pymc.io/en/latest/contributing/jupyter_style.html#authorship-and-attribution. See here for info abut the first block https://docs.pymc.io/en/latest/contributing/jupyter_style.html#first-cell

OriolAbril commented on 2022-12-28T21:42:32Z
----------------------------------------------------------------

Also, :type: is not a valid key for the metadata, it should be :category: intermediate, how-to

gbrunkhorst commented on 2022-12-29T18:33:41Z
----------------------------------------------------------------

Revised to name myself as author. Everything here is derived from somewhere, but the outline/approach (and 90% of the code and text) are original. Let me know if there are issues with that.

Removed :type:

review-notebook-app · 2022-12-28T13:58:04Z

View / edit / reply to this conversation on ReviewNB

drbenvincent commented on 2022-12-28T13:58:03Z
----------------------------------------------------------------

Ideally switch to using PyTensor and PyMC v5

gbrunkhorst commented on 2022-12-29T18:33:38Z
----------------------------------------------------------------

~~Updated and revised globally.~~ I commented prematurely. Pytensor is throwing errors for bothpymc.ODE.DifferentialEquation and pytensor.scan . Both are getting errors originating from

~\.conda\envs\pymc-dev\Lib\site-packages\pytensor\scan\scan_perform_ext.py

for pymc.ODE

ModuleNotFoundError: No module named 'scan_perform'

and for pytensor.scan

 ImportError: Scan code version mismatch

the there is another exception and a big long CompileError for each.

Let me know if you have thoughts - maybe this is getting worked on elsewhere?

gbrunkhorst commented on 2023-01-12T16:02:55Z
----------------------------------------------------------------

I had to reinstall c++ on my windows machine when I switched from aesara to pytensor. notebook currently running on PyMCv5 and Pytensor

review-notebook-app · 2022-12-28T13:58:05Z

View / edit / reply to this conversation on ReviewNB

drbenvincent commented on 2022-12-28T13:58:05Z
----------------------------------------------------------------

remove empty cell

gbrunkhorst commented on 2022-12-29T18:50:03Z
----------------------------------------------------------------

removed

OriolAbril · 2022-12-28T21:42:33Z

Also, :type: is not a valid key for the metadata, it should be :category: intermediate, how-to

View entire conversation on ReviewNB

myst_nbs/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md

OriolAbril · 2022-12-28T21:51:39Z

Agree it is busy to repeat. az.labels.NoVarLabeller() gave busy "None" text, so I cleaned it up with a few lines of matplotlib code to address the comment.

what ArviZ version are you using? I regularly use NoVarLabeller with the latest release and it works, but you might have caught a bug. It is used in https://www.pymc.io/projects/examples/en/latest/case_studies/multilevel_modeling.html?highlight=NoVarLabeller#varying-intercept-model for example.

Armavica · 2022-12-29T00:15:41Z

Ok, thank you for investigating this! Let's keep it like this then.

View entire conversation on ReviewNB

Armavica · 2022-12-29T00:17:05Z

Thank you for the changes and yes, I think that this formulation is accurate enough.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-29T18:33:39Z

Updated and revised globally.

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-29T18:33:42Z

Revised to name myself as author. Everything here is derived from somewhere, but the outline/approach (and 90% of the code and text) are original. Let me know if there are issues with that.

Removed :type:

View entire conversation on ReviewNB

gbrunkhorst · 2022-12-29T18:50:04Z

removed

View entire conversation on ReviewNB

gbrunkhorst · 2023-01-12T16:02:56Z

I had to reinstall c++ on my windows machine when I switched from aesara to pytensor. notebook currently running on PyMCv5 and Pytensor

View entire conversation on ReviewNB

gbrunkhorst · 2023-01-12T17:30:10Z

Note, we both seem to be on Arviz 0.14 - I'm not sure why az.labels.NoVarLabeller() is acting differently but suggest we forge ahead with the work around.

View entire conversation on ReviewNB

gbrunkhorst · 2023-01-12T17:33:46Z

@OriolAbril I believe that the notebook is ready for the third round of checks. Let me know if there are other fixes. Then, we can discuss what to do about the other ODE example notebooks.

OriolAbril

Thanks! I am reviewing on the myst file, but edit the jupyter notebook with the changes and then run pre-commit. If you edit the myst file you'll lose the changes

examples/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md

OriolAbril · 2023-01-15T17:45:48Z

examples/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md

+++
+
+## Authors
+Organized and rewritten by Greg Brunkhorst from multiple PyMC.io example notebooks by Sanmitra Ghosh, Demetri Pananos, and the PyMC Team.


Can you also add links to the reference pages?

We should discuss. I think 2 of the 3 referenced notebooks are obsolete and should probably be removed or heavily edited for the sake of future users. For now, I'll cross reference the one that should stay. More details:

https://www.pymc.io/projects/examples/en/latest/ode_models/ODE_with_manual_gradients.html
Suggest removing. This was a helpful notebook, but the method it shows is what pymc.ode uses under the hood, so it is redundant with later work. Also, it doesn't use autodifferentiation. The cutting edge is likely sunode or diffrax (or DifferentialEquations.jl). The part of this notebook that I think would be helpful to users is a short presentation of the second ODE example solved with DEMetropolis.

https://www.pymc.io/projects/examples/en/latest/ode_models/ODE_API_introduction.html
Suggest removing. In my new notebook, the pymc.ode module was much much slower than other methods and difficult to use. Considering the innovations since the 2019 summer of code, we probably do not want to focus on pymc.ode. The part of this notebook that I think would be helpful to users is a short presentation of the second ODE example (SIR Model) solved with DEMetropolis.

https://www.pymc.io/projects/examples/en/latest/samplers/SMC-ABC_Lotka-Volterra_example.html
This notebook has been updated and can be referenced using the sphinx cross-referencing format.

sounds good, can you open an issue to remove these two notebooks? I'll share it with the rest of the team, specially people more involved with ode related development so they can join the discussion.

examples/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md

gbrunkhorst · 2023-01-16T18:15:29Z

@OriolAbril thanks for the reviews. The latest is pushed. I checked the preview and for some reason a bunch of figures are not showing (just a path to a .....png file.) Weird. The figures are showing on my end. I'm not sure if you have seen this before. Anyway, let me know if there are other comments and I can run the notebook again with other changes.

OriolAbril · 2023-01-17T09:07:05Z

I see the images in the preview, might be a cache issue

OriolAbril

left one last comment, you'll need to rerun the whole notebook sequentially too before merging (i.e. using restart and run all)

examples/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md

gbrunkhorst · 2023-01-18T19:47:09Z

@OriolAbril thanks for the reviews - hopefully we are close! I inadvertently pushed files related to "ODE_with_manual_gradients.ipynb" by a mistake. I tried to delete them, hopefully this worked. The "ODE_Lotka_Volterra_multiple_ways.ipynb" notebook and paired ".myst.md" file are the correct ones. Sorry for the confusion.

Also, note that I want to change the number of chains in the DEMetropolis sampler to be consistent with the documention. So there will be at least one more push.

OriolAbril · 2023-01-24T23:10:13Z

Haven't forgotten about this, but I'll need to check this out locally to try and figure out why there are git conflicts and this needs a good window to sit down which hasn't happened yet.

gbrunkhorst · 2023-01-25T14:42:54Z

No problem - let me know what you find and if I can help on my end for this or future notebooks.

…ating based on the current state of the pymc examples

OriolAbril

Looks like I managed to fix the git conflicts

OriolAbril reviewed Dec 28, 2022

View reviewed changes

myst_nbs/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md Outdated Show resolved Hide resolved

twiecki requested a review from OriolAbril January 14, 2023 16:25

OriolAbril reviewed Jan 15, 2023

View reviewed changes

OriolAbril reviewed Jan 17, 2023

View reviewed changes

examples/ode_models/ODE_Lotka_Volterra_multiple_ways.myst.md Outdated Show resolved Hide resolved

gbrunkhorst added 5 commits February 2, 2023 21:19

New notebook on ODEs drawing from the previously scatter work and upd…

bb6a073

…ating based on the current state of the pymc examples

Revision pymc-devs#1 ODE Notebook based on Pymcdev comments

66fc2e0

Revision pymc-devs#2 ODE Notebook changes based on comments

e1994a2

Revision pymc-devs#3 based on comments

5437085

Revision pymc-devs#5 based on comments

3cf2825

OriolAbril force-pushed the ode_examples_update branch from 4f95fc9 to 3cf2825 Compare February 2, 2023 20:37

OriolAbril approved these changes Feb 2, 2023

View reviewed changes

OriolAbril merged commit 56a40b5 into pymc-devs:main Feb 2, 2023

New notebook on ODEs drawing from the previously scatter work and upd… #478

New notebook on ODEs drawing from the previously scatter work and upd… #478

Conversation

gbrunkhorst commented Dec 16, 2022 • edited Loading

{Insert Description}

Helpful links

review-notebook-app bot commented Dec 16, 2022

review-notebook-app bot commented Dec 18, 2022 • edited Loading

review-notebook-app bot commented Dec 18, 2022 • edited Loading

review-notebook-app bot commented Dec 18, 2022 • edited Loading

review-notebook-app bot commented Dec 18, 2022 • edited Loading

review-notebook-app bot commented Dec 18, 2022 • edited Loading

Armavica commented Dec 18, 2022

gbrunkhorst commented Dec 20, 2022

Armavica commented Dec 20, 2022 • edited Loading

review-notebook-app bot commented Dec 22, 2022 • edited Loading

review-notebook-app bot commented Dec 22, 2022 • edited Loading

review-notebook-app bot commented Dec 22, 2022 • edited Loading

review-notebook-app bot commented Dec 22, 2022 • edited Loading

review-notebook-app bot commented Dec 22, 2022 • edited Loading

gbrunkhorst commented Dec 23, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

gbrunkhorst commented Dec 27, 2022

review-notebook-app bot commented Dec 28, 2022 • edited Loading

review-notebook-app bot commented Dec 28, 2022 • edited Loading

review-notebook-app bot commented Dec 28, 2022 • edited Loading

OriolAbril commented Dec 28, 2022

OriolAbril commented Dec 28, 2022

Armavica commented Dec 29, 2022

Armavica commented Dec 29, 2022

gbrunkhorst commented Dec 29, 2022

gbrunkhorst commented Dec 29, 2022

gbrunkhorst commented Dec 29, 2022

gbrunkhorst commented Jan 12, 2023

gbrunkhorst commented Jan 12, 2023

gbrunkhorst commented Jan 12, 2023

OriolAbril left a comment

Choose a reason for hiding this comment

OriolAbril Jan 15, 2023

Choose a reason for hiding this comment

gbrunkhorst Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

OriolAbril Jan 17, 2023

Choose a reason for hiding this comment

gbrunkhorst commented Jan 16, 2023

OriolAbril commented Jan 17, 2023

OriolAbril left a comment

Choose a reason for hiding this comment

gbrunkhorst commented Jan 18, 2023 • edited Loading

OriolAbril commented Jan 24, 2023

gbrunkhorst commented Jan 25, 2023

OriolAbril left a comment

Choose a reason for hiding this comment

gbrunkhorst commented Dec 16, 2022 •

edited

Loading

review-notebook-app bot commented Dec 18, 2022 •

edited

Loading

review-notebook-app bot commented Dec 18, 2022 •

edited

Loading

review-notebook-app bot commented Dec 18, 2022 •

edited

Loading

review-notebook-app bot commented Dec 18, 2022 •

edited

Loading

review-notebook-app bot commented Dec 18, 2022 •

edited

Loading

Armavica commented Dec 20, 2022 •

edited

Loading

review-notebook-app bot commented Dec 22, 2022 •

edited

Loading

review-notebook-app bot commented Dec 22, 2022 •

edited

Loading

review-notebook-app bot commented Dec 22, 2022 •

edited

Loading

review-notebook-app bot commented Dec 22, 2022 •

edited

Loading

review-notebook-app bot commented Dec 22, 2022 •

edited

Loading

review-notebook-app bot commented Dec 28, 2022 •

edited

Loading

review-notebook-app bot commented Dec 28, 2022 •

edited

Loading

review-notebook-app bot commented Dec 28, 2022 •

edited

Loading

gbrunkhorst Jan 16, 2023 •

edited

Loading

gbrunkhorst commented Jan 18, 2023 •

edited

Loading