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

Added a more representative training loop in the training interface tutorial with early stopping #1498

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

IzicTemi
Copy link

@IzicTemi IzicTemi commented Mar 20, 2025

The training notebook only has simple training examples.
The notebook was updated to show dataset splits and a training loop with early stopping after convergence adapted from the NPE base class train() method. A simple comparison with the inbuilt train method is also shown

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @IzicTemi

can you please provide a short description of what this PR is doing?

also, please run the script tests/strip_notebook_outputs.py on your notebook and push it again, thanks!

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.09%. Comparing base (a1d7555) to head (29b2075).
Report is 13 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1498       +/-   ##
===========================================
- Coverage   89.60%   79.09%   -10.51%     
===========================================
  Files         121      122        +1     
  Lines        9357     9394       +37     
===========================================
- Hits         8384     7430      -954     
- Misses        973     1964      +991     
Flag Coverage Δ
unittests 79.09% <ø> (-10.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@IzicTemi
Copy link
Author

IzicTemi commented Mar 20, 2025

also, please run the script tests/strip_notebook_outputs.py on your notebook and push it again, thanks!

Running this made no changes to the notebook

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

thanks for the updates. I left some minor comments. However, I am having trouble to see what this PR actually changes. Can you point me to the changes you made, e.g., just posting a comment describing the changes?

If you changed the training loop in the notebook for NPE, I think it would be good to change it for NLE and NRE accordingly as well. But please double-check with @michaeldeistler

"id": "0abcbddf",
"metadata": {
"collapsed": false
},
"source": [
"Note: For the `DirectPosterior`, the batch dimension is optional, i.e., it is possible to sample for multiple observations simultaneously. Use `.sample_batch` in that case."
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please fix this typo: it should be .sample_batched

"source": [
"### Custom Data Loaders\n",
"\n",
"One helpful advantage of having access to the training loop is that you can now use your own DataLoaders during training of the density estimator. In this fashion, larger datasets can be used as input to `sbi` where `x` is potentially an image or something else. While this will require [embedding the input data](https://sbi.readthedocs.io/en/latest/tutorials/04_embedding_networks.html), a more fine grained control over loading the data is possible and allows to manage the memory requirement during training.\n",
"One helpful advantage of having access to the training loop is that you can now use your own DataLoaders during training of the density estimator. In this fashion, larger datasets can be used as input to `sbi` where `x` is potentially an image or something else. While this will require [embedding the input data](https://sbi-dev.github.io/sbi/latest/tutorials/04_embedding_networks/), a more fine grained control over loading the data is possible and allows to manage the memory requirement during training.\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this link is outdate / points the old docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems this is not fixed. Can you please fix the link @IzicTemi ?

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @IzicTemi!

Before merging this please:

  • pull the most recent main from sbi
  • checkout on your feature branch do another git merge main
  • run python tests/strip_notebook_outputs.py docs/tutorials. it will strip some outputs (I tested i locally)
  • run git add docs/tutorials/18_training_interface.ipynb to add those changes
  • push to your remote branch.
  • double check that your PR contains only your changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

the changes in this file are not part of this PR. probably due to a merge with main.

Copy link
Contributor

Choose a reason for hiding this comment

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

please follow the steps in my comment above to fix this.

"source": [
"### Custom Data Loaders\n",
"\n",
"One helpful advantage of having access to the training loop is that you can now use your own DataLoaders during training of the density estimator. In this fashion, larger datasets can be used as input to `sbi` where `x` is potentially an image or something else. While this will require [embedding the input data](https://sbi.readthedocs.io/en/latest/tutorials/04_embedding_networks.html), a more fine grained control over loading the data is possible and allows to manage the memory requirement during training.\n",
"One helpful advantage of having access to the training loop is that you can now use your own DataLoaders during training of the density estimator. In this fashion, larger datasets can be used as input to `sbi` where `x` is potentially an image or something else. While this will require [embedding the input data](https://sbi-dev.github.io/sbi/latest/tutorials/04_embedding_networks/), a more fine grained control over loading the data is possible and allows to manage the memory requirement during training.\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems this is not fixed. Can you please fix the link @IzicTemi ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants