Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tutorial Lotka Volterra multiple #1701

Merged
merged 10 commits into from
May 8, 2024

Conversation

A2P2
Copy link
Contributor

@A2P2 A2P2 commented Dec 19, 2023

The tutorial mentioned in #1450. It extends the existing example https://num.pyro.ai/en/stable/examples/ode.html by includin integration of multiple initial conditions and treating different data imperfections.
Please suggest what to fix/improve.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@fehiepsi
Copy link
Member

Sorry for the late response, @A2P2! Somehow I thought this PR is WIP. I'll review the tutorial this week.

@A2P2
Copy link
Contributor Author

A2P2 commented Jan 31, 2024

@fehiepsi No problem, take your time. I was trying to figure out why make docs failed, but was a bit stuck with pandoc.

@@ -0,0 +1,574 @@
{
Copy link
Member

@fehiepsi fehiepsi Feb 16, 2024

Choose a reason for hiding this comment

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

Maybe add link [the previous Predator-Prey Model tutorial](https://num.pyro.ai/en/stable/examples/ode.html)


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,574 @@
{
Copy link
Member

@fehiepsi fehiepsi Feb 16, 2024

Choose a reason for hiding this comment

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

Line #13.        progress_bar=False if "NUMPYRO_SPHINXBUILD" in os.environ else True,

This is unnecessary. You can set progress_bar=True.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Looks great! I just have minor comments. Could you also keep the outputs in the tutorial, so that it's easier for the reader to follow?

@@ -0,0 +1,574 @@
{
Copy link
Member

@fehiepsi fehiepsi Feb 16, 2024

Choose a reason for hiding this comment

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

Line #1.    !pip install -q numpyro@git+https://github.com/pyro-ppl/numpyro

CI failed because we need to comment out this command.

# !pip install -q ...

Reply via ReviewNB

@fehiepsi fehiepsi added this to the 0.14 milestone Feb 16, 2024
@A2P2
Copy link
Contributor Author

A2P2 commented Feb 20, 2024

Thanks for the provided feedback. I've incorporated all points.
Somehow rhats got higher than before and the committed ipynb has a json error. I'll fix it and ping you.

@A2P2
Copy link
Contributor Author

A2P2 commented Feb 21, 2024

@fehiepsi I've incorporated you feedback points, the outputs are save in the notebook now as well. A bit too many commits because I didn't know that one shouldn't use black formatting for ipynb files.

A side question, I've checked the notebook locally and in colab and the parameter estimation is quite bad locally in my case. See the screenshots below. Do you know why? Is it due to different jaxlib/python versions?

Colab:
Python 3.10.12
numpyro 0.13.2
jax 0.4.23
jaxlib 0.4.23+cuda12.cudnn89
colab
colab-plots

Locally:
Python 3.9.5
numpyro 0.13.2
jax 0.4.24
jaxlib 0.4.24
locally-correct-numpyro
locally-plots

@fehiepsi
Copy link
Member

fehiepsi commented Feb 27, 2024

Hi @A2P2, I'm not sure why. The ess looks good on my system, which has jax 0.4.21. However, it seems to me that the colab gives more reasonable results.

@fehiepsi
Copy link
Member

@A2P2 I got good results (in terms of ess and estimated mean) with max_tree_depth=10.

@fehiepsi fehiepsi removed this from the 0.14 milestone Mar 3, 2024
@A2P2
Copy link
Contributor Author

A2P2 commented Mar 5, 2024

@fehiepsi Glad it works for you. I did upload the colab version since it made more sense to me.

@fehiepsi
Copy link
Member

fehiepsi commented Mar 5, 2024

could you set max tree depth to 10? I'm seeing that it is the main reason causing low ess in colab.

@A2P2
Copy link
Contributor Author

A2P2 commented Mar 5, 2024

@fehiepsi with tree depth 10 the estimation is fairly slow. The reason I set it to 4 was to speed up the estimation a bit, while sacrificing some quality.

I would maybe leave it at 4 and add a note saying that it should be increased for better estimation quality. What do you think?

image

@fehiepsi
Copy link
Member

fehiepsi commented Mar 12, 2024

@A2P2 The following settings seem to work very well (not slow) in my system

n_datasets = 3
odeint_with_kwargs = functools.partial(
        odeint, rtol=1e-6, atol=1e-5, mxstep=1000,
    )
max_tree_depth=10,

image

Using n_datasets=3 also makes the plots clearer.

@fehiepsi
Copy link
Member

fehiepsi commented May 6, 2024

Hi @A2P2 - We are going to make a new release soon. As discussed above, the content looks great. We just need to adjust a couple of settings to get more consistent results and to visualize better. Do you want to incorporate those small changes before the release?

@A2P2
Copy link
Contributor Author

A2P2 commented May 6, 2024 via email

@A2P2
Copy link
Contributor Author

A2P2 commented May 6, 2024

@fehiepsi I've incorporated your latest suggestions. Thanks for finding better ODE solver parameters. I just forgot that I've tuned it to so small values. Let me know if there is anything else.

@fehiepsi
Copy link
Member

fehiepsi commented May 7, 2024

@A2P2 Could you run make format to pass the lint checks? It seems that there are many unused imports.

@A2P2
Copy link
Contributor Author

A2P2 commented May 7, 2024

@fehiepsi formatted now. I had the old version of the Makefile with black there, formatted with ruff now.

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Thanks, @A2P2!

@fehiepsi fehiepsi merged commit 2b85765 into pyro-ppl:master May 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants