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

1d and 2d cnn embedding nets. #751

Merged
merged 3 commits into from
Nov 3, 2022
Merged

1d and 2d cnn embedding nets. #751

merged 3 commits into from
Nov 3, 2022

Conversation

janfb
Copy link
Contributor

@janfb janfb commented Oct 19, 2022

failing test for 1D CNN embedding, see #750

and a new implementation of the CNN embedding net including 1D and 2D convolution, and some input checks.

Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

The test looks good, thanks! But I guess we do not merge this yet, right?

@janfb
Copy link
Contributor Author

janfb commented Oct 24, 2022

yes, we should fix the 1D embedding net with asserts and find hyperparams that work for the common scenarios.

@janfb janfb force-pushed the fix-cnn-embedding branch 2 times, most recently from 1d9de7d to 3f70e81 Compare November 2, 2022 17:48
@janfb janfb changed the title test for 1D cnn embedding. 1d and 2d cnn embedding nets. Nov 2, 2022
Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Nice, thanks for taking care of this! A few comments below.

sbi/neural_nets/embedding_nets.py Outdated Show resolved Hide resolved
sbi/neural_nets/embedding_nets.py Outdated Show resolved Hide resolved
sbi/neural_nets/embedding_nets.py Show resolved Hide resolved
sbi/neural_nets/embedding_nets.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #751 (0d66e3a) into main (fda9d4e) will increase coverage by 0.35%.
The diff coverage is 100.00%.

❗ Current head 0d66e3a differs from pull request most recent head 0f8dbf5. Consider uploading reports for the commit 0f8dbf5 to get more accurate results

@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   74.13%   74.48%   +0.35%     
==========================================
  Files          78       77       -1     
  Lines        6081     6111      +30     
==========================================
+ Hits         4508     4552      +44     
+ Misses       1573     1559      -14     
Flag Coverage Δ
unittests 74.48% <100.00%> (+0.35%) ⬆️

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

Impacted Files Coverage Δ
sbi/neural_nets/embedding_nets.py 91.42% <100.00%> (+31.01%) ⬆️
sbi/utils/restriction_estimator.py 76.79% <0.00%> (-6.33%) ⬇️
sbi/samplers/rejection/rejection.py 87.05% <0.00%> (-2.95%) ⬇️
sbi/utils/__init__.py 100.00% <0.00%> (ø)
sbi/inference/posteriors/direct_posterior.py 98.21% <0.00%> (ø)
sbi/samplers/__init__.py
sbi/analysis/sbc.py 80.00% <0.00%> (+1.33%) ⬆️
sbi/inference/posteriors/mcmc_posterior.py 84.83% <0.00%> (+2.80%) ⬆️
sbi/inference/snre/snre_base.py 94.87% <0.00%> (+3.41%) ⬆️
sbi/inference/base.py 85.61% <0.00%> (+7.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Good to go once documentation is finalized.

sbi/neural_nets/embedding_nets.py Outdated Show resolved Hide resolved
@janfb janfb merged commit a0757d4 into main Nov 3, 2022
@janfb janfb deleted the fix-cnn-embedding branch November 3, 2022 11:19
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