issue #568 : small embedding net as default #624

JuliaLinhart · 2022-01-28T10:23:29Z

There @michaeldeistler, should work now!

michaeldeistler · 2022-01-28T11:16:12Z

Great, thanks!

michaeldeistler · 2022-01-28T11:16:38Z

@janfb is this ok to merge for you? The PR adds a one-layer neural network as default embedding net

michaeldeistler

Ok unfortunately tests are failing. It seems that the net does not infer the shape of x correctly

janfb

Looks good, thanks for tackling this!
One remark: what is the overhead of this? What the difference in training speed of the default settings, e.g., NPE with maf, or NRE with resnet (since it uses two embeddings), with and without the new default embedding?
Could we run a small benchmark, just to get an impression?

JuliaLinhart · 2022-01-28T12:20:43Z

What about now?

michaeldeistler · 2022-01-28T13:09:53Z

Just started running them. You can also run them locally with pytest -m "not slow and not gpu" tests/

michaeldeistler · 2022-01-28T13:39:04Z

still failing unfortunately

JuliaLinhart · 2022-01-28T18:40:39Z

I fixed it, but it's not so pretty.. You might want to have a look! The remaining test failures are linked to performance issues in check_c2st...

JuliaLinhart · 2022-01-30T18:55:26Z

I don't know why, but checking the type with type(embedding_net) did not work. It always seemed to be True as the embedding net turned out to be DefaultEmbeddingNet even when trying to use another embedding net (e.g. nn.Identity() or a CNN).

michaeldeistler

I think the current solution is ok. I left some comments that should make it a bit more readable.

sbi/utils/sbiutils.py

sbi/neural_nets/classifier.py

michaeldeistler · 2022-01-31T12:31:49Z

Looks good now, thanks! Have you ran a small experiment to compare the runtimes with and without DefaultEmbedding?

JuliaLinhart · 2022-01-31T13:17:37Z

No, not yet! Any suggestions which benchmarking example I should use for that and where I should implement that? Also, how do you measure the runtimes, just to be on the same page.

michaeldeistler · 2022-01-31T15:56:04Z

you can just use a linear gaussian as done here.

Only the training runtime matters, you can measure it e.g. with time.time() here

michaeldeistler · 2022-01-31T15:57:48Z

My bigger concern is that some tests are failing because of their c2st right now. I'm a bit worried that the default embedding net impedes performance for small models such as the ones we run in our tests. If that is the case, we should reconsider using a DefaultEmbeddingNet. Could you run slow tests with pytest -m 'not gpu' tests and check how many fail?

JuliaLinhart · 2022-02-01T08:33:57Z

It's been running all night and seems to be stuck on a code in linearGaussian_snpe_test.py, is it supposed to take that long?

michaeldeistler · 2022-02-01T09:45:16Z

Hm, no, the whole thing should take about 2-3 hours. Not sure what's going wrong here. You can also run specific tests with
pytest -m 'not gpu' tests/linear_gaussian_snre_test.py

JuliaLinhart · 2022-02-01T11:03:12Z

Still bugging on the linear_gaussian_snpe_test.py but I manually ran the others:

janfb · 2022-02-01T11:47:10Z

I am able to reproduce the failing tests for pytest -m "not gpu" tests/linearGaussian_snpe_test.py. Changing num_simulations = 2500 to 2600 makes them pass again.
Just speculating here, but it could be that the increased number of parameters induced by the embedding net just needs more training data than before?

JuliaLinhart · 2022-02-01T17:12:19Z

Nice catch! I did the same for the others and the tests pass now!

janfb · 2022-02-02T07:39:34Z

ok cool!
how about the slow tests? For me
pytest -m "not gpu" tests/linearGaussian_snpe_test.py::test_sample_conditional
is stuck in tuning the bracket widths. On main it passes in 3min. This seems to be due to the embedding net.

Plus, there is a merge conflict insbi/neural_nets/mdn.py, due to the GPU PR. So this needs another local rebase.

JuliaLinhart · 2022-02-02T08:57:56Z

Yeah for me it's stuck too! And the merge conflict is because someone added checks in the mdn.py file. I didn't do anything because I didn't want to mess things up haha But basically we just have to add a line for imports

janfb · 2022-02-02T09:02:13Z

Update @JuliaLinhart : setting max_num_epochs=60 in line 432 should do the trick.

JuliaLinhart · 2022-02-02T09:44:41Z

I'm running the slow tests again now

JuliaLinhart · 2022-02-02T17:50:58Z

On my side all tests pass now

codecov-commenter · 2022-02-06T19:38:47Z

Codecov Report

Merging #624 (edc4183) into main (d373ff6) will increase coverage by 1.97%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #624      +/-   ##
==========================================
+ Coverage   68.72%   70.69%   +1.97%     
==========================================
  Files          67       68       +1     
  Lines        4476     4713     +237     
==========================================
+ Hits         3076     3332     +256     
+ Misses       1400     1381      -19

Flag	Coverage Δ
unittests	`70.69% <100.00%> (+1.97%)`	⬆️

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

Impacted Files	Coverage Δ
sbi/neural_nets/classifier.py	`100.00% <100.00%> (ø)`
sbi/neural_nets/flow.py	`88.17% <100.00%> (+0.96%)`	⬆️
sbi/neural_nets/mdn.py	`100.00% <100.00%> (ø)`
sbi/utils/get_nn_models.py	`85.71% <100.00%> (+0.29%)`	⬆️
sbi/utils/sbiutils.py	`79.03% <100.00%> (+0.88%)`	⬆️
sbi/utils/__init__.py	`100.00% <0.00%> (ø)`
sbi/analysis/__init__.py	`100.00% <0.00%> (ø)`
sbi/utils/posterior_ensemble.py	`85.29% <0.00%> (ø)`
sbi/inference/posteriors/base_posterior.py	`81.81% <0.00%> (+1.51%)`	⬆️
sbi/inference/posteriors/mcmc_posterior.py	`77.46% <0.00%> (+4.22%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d373ff6...edc4183. Read the comment docs.

janfb

Thanks a lot, it looks good to me now and tests are passing!
One final question: for some test cases you increased the number of simulations quite a lot, is this necessary? E.g., are these number lower bounds on making the tests pass?

tests/linearGaussian_snle_test.py

tests/sbiutils_test.py

janfb · 2022-02-07T14:11:42Z

Alright, we can merge this into main now. The final thing to do @JuliaLinhart would be a rebase on main to resolve the conflicts.
It seems you have your changes on the main of your fork, not on a feature branch, so this requires a bit of wiggling. But maybe you could try to just rebase on the upstream main and resolve the conflicts:
(assuming your fork is in origin and the sbi repo is in upstream)

git pull upstream main --rebase

then resolve the conflict, I think it is just one additional import in neural_nets.mdn
add the fixed file with git add
git rebase --continue
force push to your fork main to include the rebase:git push origin main -f

JuliaLinhart · 2022-02-07T14:24:21Z

Yes I forgot to create a branch.. 🤭
But it seems that there are not conflicts, I solved them a few days ago! or am I looking at the wrong place?

janfb · 2022-02-07T14:30:11Z

no problem :) but there is one conflict, see below "This branch cannot be rebased due to conflicts".

You probably have to rebase again on the most recent main.

JuliaLinhart · 2022-02-07T15:18:35Z

hmm I really cannot see that..

(base) julialinhart@nat-eduroam-138-gw-01-sif sbi % git pull upstream main --rebase
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

(base) julialinhart@nat-eduroam-138-gw-01-sif sbi % git branch -vv
* main 68c744a [origin/main] lower bound for num_simulations

(base) julialinhart@nat-eduroam-138-gw-01-sif sbi % git pull --rebase              
Already up to date.
(base) julialinhart@nat-eduroam-138-gw-01-sif sbi % git rebase --continue
fatal: No rebase in progress?
(base) julialinhart@nat-eduroam-138-gw-01-sif sbi % git rebase          
Current branch main is up to date.

janfb · 2022-02-07T15:20:59Z

it seems you haven't set upstream correctly. maybe

git remote add upstream https://github.com/mackelab/sbi.git

helps?

…lity

…ngNet

michaeldeistler approved these changes Jan 28, 2022

View reviewed changes

michaeldeistler mentioned this pull request Jan 28, 2022

issue #568: small embedding net as default #623

Closed

michaeldeistler linked an issue Jan 28, 2022 that may be closed by this pull request

Small embedding_net as default? #568

Closed

michaeldeistler self-requested a review January 28, 2022 11:45

michaeldeistler requested changes Jan 28, 2022

View reviewed changes

janfb reviewed Jan 28, 2022

View reviewed changes

michaeldeistler requested changes Jan 31, 2022

View reviewed changes

janfb reviewed Feb 7, 2022

View reviewed changes

tests/linearGaussian_snle_test.py Show resolved Hide resolved

tests/sbiutils_test.py Show resolved Hide resolved

Julia Linhart and others added 10 commits February 7, 2022 16:22

issue sbi-dev#568 : small embedding net as default

7debc13

changes to resolve test-failures

3f13e95

initialization of DefaultEmbeddingNet (inferred dimensionality)

c1619d5

check if embedding net is DefaultEmbeddingNet for inferred dimensiona…

5077313

…lity

build_network method

89332f2

more simulations in tests to resolve c2est issues with DefaultEmbeddi…

40ced2a

…ngNet

black formatting

232c1eb

changes to make tests pass

01566a6

one more change to make snpe test pass

2f8cda2

lower bound for num_simulations

9ddd73f

JuliaLinhart force-pushed the main branch from 68c744a to 9ddd73f Compare February 7, 2022 15:29

janfb merged commit 51d364f into sbi-dev:main Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #568 : small embedding net as default #624

issue #568 : small embedding net as default #624

JuliaLinhart commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

michaeldeistler left a comment

janfb left a comment

JuliaLinhart commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

JuliaLinhart commented Jan 28, 2022 •

edited

Loading

JuliaLinhart commented Jan 30, 2022

michaeldeistler left a comment

michaeldeistler commented Jan 31, 2022

JuliaLinhart commented Jan 31, 2022 •

edited

Loading

michaeldeistler commented Jan 31, 2022

michaeldeistler commented Jan 31, 2022

JuliaLinhart commented Feb 1, 2022 •

edited

Loading

michaeldeistler commented Feb 1, 2022

JuliaLinhart commented Feb 1, 2022 •

edited

Loading

janfb commented Feb 1, 2022 •

edited

Loading

JuliaLinhart commented Feb 1, 2022

janfb commented Feb 2, 2022 •

edited

Loading

JuliaLinhart commented Feb 2, 2022 •

edited

Loading

janfb commented Feb 2, 2022 •

edited

Loading

JuliaLinhart commented Feb 2, 2022

JuliaLinhart commented Feb 2, 2022

codecov-commenter commented Feb 6, 2022

janfb left a comment

janfb commented Feb 7, 2022

JuliaLinhart commented Feb 7, 2022 •

edited

Loading

janfb commented Feb 7, 2022

JuliaLinhart commented Feb 7, 2022 •

edited

Loading

janfb commented Feb 7, 2022

issue #568 : small embedding net as default #624

issue #568 : small embedding net as default #624

Conversation

JuliaLinhart commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

michaeldeistler left a comment

Choose a reason for hiding this comment

janfb left a comment

Choose a reason for hiding this comment

JuliaLinhart commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

michaeldeistler commented Jan 28, 2022

JuliaLinhart commented Jan 28, 2022 • edited Loading

JuliaLinhart commented Jan 30, 2022

michaeldeistler left a comment

Choose a reason for hiding this comment

michaeldeistler commented Jan 31, 2022

JuliaLinhart commented Jan 31, 2022 • edited Loading

michaeldeistler commented Jan 31, 2022

michaeldeistler commented Jan 31, 2022

JuliaLinhart commented Feb 1, 2022 • edited Loading

michaeldeistler commented Feb 1, 2022

JuliaLinhart commented Feb 1, 2022 • edited Loading

janfb commented Feb 1, 2022 • edited Loading

JuliaLinhart commented Feb 1, 2022

janfb commented Feb 2, 2022 • edited Loading

JuliaLinhart commented Feb 2, 2022 • edited Loading

janfb commented Feb 2, 2022 • edited Loading

JuliaLinhart commented Feb 2, 2022

JuliaLinhart commented Feb 2, 2022

codecov-commenter commented Feb 6, 2022

Codecov Report

janfb left a comment

Choose a reason for hiding this comment

janfb commented Feb 7, 2022

JuliaLinhart commented Feb 7, 2022 • edited Loading

janfb commented Feb 7, 2022

JuliaLinhart commented Feb 7, 2022 • edited Loading

janfb commented Feb 7, 2022

JuliaLinhart commented Jan 28, 2022 •

edited

Loading

JuliaLinhart commented Jan 31, 2022 •

edited

Loading

JuliaLinhart commented Feb 1, 2022 •

edited

Loading

JuliaLinhart commented Feb 1, 2022 •

edited

Loading

janfb commented Feb 1, 2022 •

edited

Loading

janfb commented Feb 2, 2022 •

edited

Loading

JuliaLinhart commented Feb 2, 2022 •

edited

Loading

janfb commented Feb 2, 2022 •

edited

Loading

JuliaLinhart commented Feb 7, 2022 •

edited

Loading

JuliaLinhart commented Feb 7, 2022 •

edited

Loading