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

Zuko density estimators (#1088) #1116

Merged
merged 16 commits into from
Apr 5, 2024
Merged

Conversation

anastasiakrouglova
Copy link
Contributor

@anastasiakrouglova anastasiakrouglova commented Mar 27, 2024


What does this implement/fix? Explain your changes

Provides Zuko density estimators and resolves comments from previous PR.

Does this close any currently open issues?

Fixes # (issue)

Any relevant code examples, logs, error output, etc?

Up to date with latest comments

Any other comments?

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • I have read and understood the contribution
    guidelines
  • I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have reported how long the new tests run and potentially marked them
    with pytest.mark.slow.
  • New and existing unit tests pass locally with my changes
  • I performed linting and formatting as described in the contribution
    guidelines
  • I rebased on main (or there are no conflicts with main)

* update zuko to 1.1.0

* test zuko_gmm commit

* build_zuko_nsf added

* add build_zuko_naf, update test

* add license change to pr template.

* CLN pyproject.toml (#1009)

* CLN pyproject.toml

* CLN optional deps comment

* CLN alphabetical order

* fix x_o and broken link tutorial 7 (#1003)

* fix x_o and broken link tutorial 7

* typo in title

* suppress plotting output

---------

Co-authored-by: Matthijs <matthijs@example.com>

* replace prepare_for_sbi in tutorials (#1013)

* add zuko density estimators

* not working gmm

* update tests for PR

* update PR for pyright

* resolve pyright

* add reportArgumentType

* resolve pyright issue

* resolve all issues pyright

* resolve pyright

* add typing and docstring

* add functions from factory to test

* remove comment mdn file

* add docstrings flow file

* add docstring in density_estimator_test.py

* Update sbi/neural_nets/flow.py

Co-authored-by: Sebastian Bischoff <sebastian@salzreute.de>

* Update sbi/neural_nets/flow.py

Co-authored-by: Sebastian Bischoff <sebastian@salzreute.de>

* Update sbi/neural_nets/flow.py

Co-authored-by: Sebastian Bischoff <sebastian@salzreute.de>

* removed pyright

---------

Co-authored-by: bkmi <12955549+bkmi@users.noreply.github.com>
Co-authored-by: Nastya Krouglova <nastyakrouglova@Nastyas-MacBook-Pro.local>
Co-authored-by: Jan Boelts <jan.boelts@mailbox.org>
Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>
Co-authored-by: Matthijs Pals <34062419+Matthijspals@users.noreply.github.com>
Co-authored-by: Matthijs <matthijs@example.com>
Co-authored-by: zinaStef <49067201+zinaStef@users.noreply.github.com>
Co-authored-by: Sebastian Bischoff <sebastian@salzreute.de>
@Baschdl
Copy link
Contributor

Baschdl commented Mar 28, 2024

@anastasiakrouglova Could you resolve the merge conflicts with main? This should also trigger the tests to run.

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 a lot for adding all the flows! 🔥

I added a couple of suggestions, please have a look. Happy to discuss them if needed.

sbi/neural_nets/factory.py Outdated Show resolved Hide resolved
sbi/neural_nets/factory.py Outdated Show resolved Hide resolved
sbi/neural_nets/flow.py Outdated Show resolved Hide resolved
sbi/neural_nets/flow.py Show resolved Hide resolved
tests/neural_nets_factory.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 84.04255% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 77.20%. Comparing base (7be2115) to head (583671d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1116      +/-   ##
==========================================
- Coverage   85.45%   77.20%   -8.25%     
==========================================
  Files          90       89       -1     
  Lines        6612     6610       -2     
==========================================
- Hits         5650     5103     -547     
- Misses        962     1507     +545     
Flag Coverage Δ
unittests 77.20% <84.04%> (-8.25%) ⬇️

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

Files Coverage Δ
sbi/neural_nets/density_estimators/zuko_flow.py 78.94% <100.00%> (ø)
sbi/neural_nets/factory.py 89.47% <71.42%> (-2.06%) ⬇️
sbi/neural_nets/flow.py 92.97% <84.70%> (-5.65%) ⬇️

... and 23 files with indirect coverage changes

@anastasiakrouglova
Copy link
Contributor Author

Thank you for the feedback. I finally removed the CNF zuko flow, but all the rest is implemented.

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.

Great, thanks!

I added three last comments.

sbi/neural_nets/factory.py Outdated Show resolved Hide resolved
sbi/neural_nets/factory.py Show resolved Hide resolved
tutorials/05_embedding_net.ipynb Outdated Show resolved Hide resolved
@janfb janfb self-assigned this Apr 4, 2024
anastasiakrouglova and others added 2 commits April 4, 2024 15:54
Co-authored-by: Jan <janfb@users.noreply.github.com>
@anastasiakrouglova anastasiakrouglova requested a review from janfb April 4, 2024 14:17
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.

One last issue about the changes in the embedding net tutorial notebook.

tutorials/05_embedding_net.ipynb Outdated Show resolved Hide resolved
@janfb
Copy link
Contributor

janfb commented Apr 5, 2024

Indeed, it's quite confusing. These changes to the 05_embedding_net notebook are new, where do they come from?

I can try to fix it by explicitly removing those notebook changes from this PR and move them to a separate branch as a backup.

@anastasiakrouglova
Copy link
Contributor Author

anastasiakrouglova commented Apr 5, 2024

Okay that would be awesome, thank you! I'm not sure where the issues come from. At first, I was accidentally merging with the main of the forked bkmi/sbi repository. So maybe it came from this place?

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.

Alright, now we are good!
(The notebook changes were duplicates or an older version of that notebook that was merged in on the way, so it was fine to discard them.)

Thanks so much again for adding all the zuko flows - great work! 👏

@janfb janfb merged commit cbf9dca into main Apr 5, 2024
5 checks passed
@janfb janfb deleted the 974-zuko-density-estimators-interface branch April 5, 2024 09:51
@anastasiakrouglova
Copy link
Contributor Author

Great!! Thank you so much for reviewing it thoroughly!

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.

3 participants