Add a forward-only task to sbibm by psteinb · Pull Request #19 · sbi-benchmark/sbibm

psteinb · 2021-10-01T15:16:04Z

this PR paves the way to allow users to add simulations without a reference posterior. This way, (I hope) it becomes easier for users to test drive and benchmark sbi for their use case even.

Closes #19

psteinb · 2021-10-01T15:17:09Z

I am not yet 100% happy with the naming scheme. I also observed that no tasks appear to have unit tests.

jan-matthis · 2021-10-04T10:46:31Z

Thanks for starting off a PR! Perhaps it would be best to have a general unit tests that checks all tasks in the same way and consider more specific ones on a per-task basis?

- renamed classes and files accordingly - added low level tests to know that the pytorch/pyro api is used correctly

psteinb · 2021-11-02T16:42:29Z

added a very general test suite for two_moons (a starting point for more perhaps)
finished norefposterior implementation for the time being
add unit tests to illustrate usage limitations

psteinb · 2021-11-03T16:54:32Z

The code is pretty much done at this point. the simulate step for norefposterior turns out to be quite slow. I am not a pyro expert and hence need to check at some later point in time, what the root cause of this might be.

@jan-matthis please review.

- the covariance matrix for MultivariateNormal is constructed in such a fashion, that it cannot be anything else than positive semi-definite - keeping `validate_args=True` produced a tremendous slowdown (4x and more)

psteinb · 2021-11-04T16:22:57Z

alright, this PR is ready to go from my side. Studying the other tasks, I see that my application tends to be somewhat special as I simulate an entire dense representation. So I am happy to receive any feedback on that.

I'll check now, how to obtain benchmark results on this task.

jan-matthis · 2021-11-04T18:57:16Z

Great, I will review it tomorrow

jan-matthis

I left some first comments on the PR

sbibm/tasks/__init__.py

sbibm/tasks/norefposterior/task.py

jan-matthis · 2021-11-05T15:57:30Z

tests/tasks/test_norefposterior.py

@@ -0,0 +1,402 @@
+import numpy as np


Great! Can we factor some of these tests out into a separate PR that adds generic tests for all tasks?

Can you make a concrete proposal?
My suggestion: I am happy to send another PR for test_two_moons.py while keeping test_norefposterior.py.
What do you think?

I will hopefully have time to work on the tests and #23 tomorrow, or Wednesday latest, and make a concrete proposal then. Cheers

Cheers for opening #29 -- I'd suggest we parametrize those tests to run for all tasks, that's what I had in mind when I asked about "generic tests" above

ok, I propose to merge this first and then adopt it in #29 or a subsequent PR.

tests/tasks/test_two_moons.py

…posterior exists

jan-matthis · 2021-11-10T15:01:55Z

Could you rebase on main for the next round of reviews?

psteinb · 2021-11-10T15:09:05Z

huh, I thought I did so last night. Let me check.

psteinb · 2021-11-10T15:25:05Z

Done, I hope.

jan-matthis

Looks great overall, only a few comments

sbibm/tasks/noref_beam/task.py

tests/tasks/test_noref_beam.py

psteinb · 2021-11-11T10:25:49Z

Sorry folks, I hand-cuffed myself by using git rebase in a wrong fashion. I undid my rebase from earlier now (using cherry-pick) and repeated the exercise.

psteinb · 2021-11-11T11:36:42Z

I hope I have addressed all comments now. Sorry for the mess with the accidental rebase.

jan-matthis · 2021-11-11T13:40:13Z

No worries at all -- unfortunately, I think something is not quite right yet: Currently the PR still contains changes are on the main branch already

psteinb · 2021-11-11T15:50:25Z

Not sure what you mean. github says this PR has no conflicts. I just did a manual git merge upstream main ... what am I missing?

jan-matthis · 2021-11-11T20:25:13Z

I think you resolved the issue with commit b876c91 above. All looks good now!

I assume this is ready to be merged then -- or should we rather do #29 first? 🎉

psteinb · 2021-11-12T16:11:58Z

Let's wait for #29 to be merged first. Then I will close this PR, split the code according to #29 and resubmit a new PR. One merit of this is also, that this very long conversation gets squashed into a handful of commits. Feel free to disagree.

Quite frankly: I'd love to get on with stuff and start running experiments.

jan-matthis · 2021-11-12T16:24:29Z

#29 is merged -- so only this one is left -- almost done

jan-matthis · 2021-11-12T16:35:39Z

Let's wait for #29 to be merged first. Then I will close this PR, split the code according to #29 and resubmit a new PR. One merit of this is also, that this very long conversation gets squashed into a handful of commits. Feel free to disagree.

Sounds good to me!

psteinb · 2021-11-12T17:04:37Z

closed in favor of #34
Thanks to @jan-matthis and @janfb for all your valuable feedback

psteinb added 3 commits October 1, 2021 17:11

unit tests for forward only task

4fadca4

added forward task for projected multivariate normal distribution

4171991

registered forward simulation with infrastructure

bed2c1a

psteinb added 10 commits October 20, 2021 23:56

renamed and refactored

ba79835

- renamed classes and files accordingly - added low level tests to know that the pytorch/pyro api is used correctly

clear labelling of test sections

ea16de8

refactoring boilerplate code into helper functions

1478e75

isort on tests

3f6d0ff

black on new code

967bdb3

added default observations

4e1b7ce

reference tests for two_moons

b3e956c

norefposterior rejabc tests implemented

3fa061b

motivation for tests

80d2731

document expectations for tests

fc631d5

psteinb added 5 commits November 3, 2021 15:58

test that reference prior exists

506de31

added tests to demo benchmark

fb1b0b1

trying to reduce simulation runtime

e3f095e

detaching the coordinate mesh

591dc06

demo how to change task params

ce6a905

psteinb marked this pull request as ready for review November 3, 2021 16:54

psteinb added 3 commits November 3, 2021 17:55

isort and black ran on new code

f7274c9

ditch validation of the MultivariateNormal input parameters

c5a39fa

- the covariance matrix for MultivariateNormal is constructed in such a fashion, that it cannot be anything else than positive semi-definite - keeping `validate_args=True` produced a tremendous slowdown (4x and more)

using pyro utilities to fix the seed

350e438

psteinb mentioned this pull request Nov 4, 2021

Black isort #22

Closed

jan-matthis reviewed Nov 5, 2021

View reviewed changes

janfb and others added 5 commits November 9, 2021 15:38

Add mean and variance metrics (sbi-benchmark#11)

212c05f

Update changelog

1cf9a7d

Code formatting

124e033

changed name to something more specific indicating that no reference …

9fda640

…posterior exists

CamelCase and type annotations in function signatures

ccc17d4

psteinb mentioned this pull request Nov 9, 2021

Tests for task's API #29

Merged

jan-matthis linked an issue Nov 10, 2021 that may be closed by this pull request

Adding a forward-only task #18

Open

jan-matthis reviewed Nov 10, 2021

View reviewed changes

sbibm/tasks/noref_beam/task.py Outdated Show resolved Hide resolved

sbibm/tasks/noref_beam/task.py Outdated Show resolved Hide resolved

tests/tasks/test_noref_beam.py Show resolved Hide resolved

Adapt new ABC API of sbi, outsource KDE entirely (sbi-benchmark#26)

365ee41

psteinb force-pushed the forward-only-task branch 2 times, most recently from d5ef8a5 to 365ee41 Compare November 11, 2021 10:25

psteinb added 4 commits November 11, 2021 12:12

more docstrings and and tests

4340644

moving comment into docstring

ab6aa26

using pyro.sample

05f0d4c

removed test of two_moons in favor of sbi-benchmark#29

a676e65

Merge remote-tracking branch 'upstream/main' into forward-only-task

b876c91

psteinb mentioned this pull request Nov 12, 2021

First noreference posterior task #34

Open

psteinb closed this Nov 12, 2021

Conversation

psteinb commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psteinb commented Oct 1, 2021

Uh oh!

jan-matthis commented Oct 4, 2021

Uh oh!

psteinb commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psteinb commented Nov 3, 2021

Uh oh!

psteinb commented Nov 4, 2021

Uh oh!

jan-matthis commented Nov 4, 2021

Uh oh!

jan-matthis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jan-matthis Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

psteinb Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

jan-matthis Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

jan-matthis Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

psteinb Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jan-matthis commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psteinb commented Nov 10, 2021

Uh oh!

psteinb commented Nov 10, 2021

Uh oh!

jan-matthis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psteinb commented Nov 11, 2021

Uh oh!

psteinb commented Nov 11, 2021

Uh oh!

jan-matthis commented Nov 11, 2021

Uh oh!

psteinb commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-matthis commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psteinb commented Nov 12, 2021

Uh oh!

jan-matthis commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-matthis commented Nov 12, 2021

Uh oh!

psteinb commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

psteinb commented Oct 1, 2021 •

edited

Loading

psteinb commented Nov 2, 2021 •

edited

Loading

jan-matthis commented Nov 10, 2021 •

edited

Loading

psteinb commented Nov 11, 2021 •

edited

Loading

jan-matthis commented Nov 11, 2021 •

edited

Loading

jan-matthis commented Nov 12, 2021 •

edited

Loading