-
Notifications
You must be signed in to change notification settings - Fork 244
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
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Sorry for the late response, @A2P2! Somehow I thought this PR is WIP. I'll review the tutorial this week. |
@fehiepsi No problem, take your time. I was trying to figure out why |
@@ -0,0 +1,574 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add link [t
he previous Predator-Prey Model tutorial](https://num.pyro.ai/en/stable/examples/ode.html)
Reply via ReviewNB
There was a problem hiding this comment.
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 @@ | |||
{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this 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 @@ | |||
{ |
There was a problem hiding this comment.
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
Thanks for the provided feedback. I've incorporated all points. |
This reverts commit 3e4ac2a. revert black formatting
@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: Locally: |
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. |
@A2P2 I got good results (in terms of ess and estimated mean) with |
@fehiepsi Glad it works for you. I did upload the colab version since it made more sense to me. |
could you set max tree depth to 10? I'm seeing that it is the main reason causing low ess in colab. |
@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? |
@A2P2 The following settings seem to work very well (not slow) in my system n_datasets = 3
Using n_datasets=3 also makes the plots clearer. |
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? |
My bad, thanks for reminding. Will do today or tomorrow.On 6 May 2024, at 21:05, Du Phan ***@***.***> wrote:
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?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@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. |
@A2P2 Could you run make format to pass the lint checks? It seems that there are many unused imports. |
@fehiepsi formatted now. I had the old version of the Makefile with black there, formatted with ruff now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @A2P2!
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.