sbi-benchmark / sbibm

Simulation-based inference benchmark
https://sbi-benchmark.github.io
MIT License
88 stars 34 forks source link

Add a forward-only task to sbibm #19

Closed psteinb closed 2 years ago

psteinb commented 2 years ago

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 commented 2 years ago

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

jan-matthis commented 2 years ago

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?

psteinb commented 2 years ago
psteinb commented 2 years ago

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.

psteinb commented 2 years ago

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 commented 2 years ago

Great, I will review it tomorrow

jan-matthis commented 2 years ago

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

psteinb commented 2 years ago

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

psteinb commented 2 years ago

Done, I hope.

psteinb commented 2 years ago

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 commented 2 years ago

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

jan-matthis commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jan-matthis commented 2 years ago

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 commented 2 years ago

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