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

[gbrunkhorst]

…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[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[Armavica]

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]

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]

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[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Revised for next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[gbrunkhorst]

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

[gbrunkhorst]

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]

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]

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]

Revised for next pull request.

---

View entire conversation on ReviewNB

[gbrunkhorst]

Revised for next pull request.

---

View entire conversation on ReviewNB

[gbrunkhorst]

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]

formatting turned off in next pull request.

---

View entire conversation on ReviewNB

[gbrunkhorst]

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]

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]

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[bot]]

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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[bot] commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

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 commented 1 year ago

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

---

View entire conversation on ReviewNB

Armavica commented 1 year ago

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

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

Updated and revised globally.

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

removed

---

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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

gbrunkhorst commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[OriolAbril]

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

---

View entire conversation on ReviewNB

[OriolAbril]

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]

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

---

View entire conversation on ReviewNB

[Armavica]

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

---

View entire conversation on ReviewNB

[gbrunkhorst]

Updated and revised globally.

---

View entire conversation on ReviewNB

[gbrunkhorst]

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]

removed

---

View entire conversation on ReviewNB

[gbrunkhorst]

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]

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]

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

[gbrunkhorst]

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

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

[gbrunkhorst]

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

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]

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