sbi-benchmark / sbibm

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

Adapt (S)MC-ABC API to latest `sbi` version #30

Closed psteinb closed 2 years ago

psteinb commented 2 years ago

the mcabc API changed with sbi 0.17.1 ... it occurred to me that this was not propagated correctly to sbibm

psteinb commented 2 years ago

btw, I could add a unit test for the run method to this PR. I saw there is none at the moment.

psteinb commented 2 years ago

See als https://github.com/mackelab/sbi/blob/17591339b47854c57e999e6fe4714d073379a58f/tests/abc_test.py#L53

janfb commented 2 years ago

thanks for fixing this! LGTM @jan-matthis

jan-matthis commented 2 years ago

Great, thanks for fixing this!

Regarding ABC tests, I'm considering doing that in the sbi repo only, now that we moved them there. What do you think?

psteinb commented 2 years ago

Sure thing, I just had in mind to add an interface test, that is all. Just in case sbibm gets in trouble with sbi versions in the future.

jan-matthis commented 2 years ago

I see, yes -- would be good to have a small test to check whether it runs at all (without asserting correctness of results etc.)

psteinb commented 2 years ago

Cool, I’ll try to do that tomorrow.

On 10. Nov 2021, at 16:15, jan-matthis @.***> wrote:

 I see, yes -- would be good to have a small test to check whether it runs at all (without asserting correctness of results etc.)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

psteinb commented 2 years ago

I added some simple code, which produces an error from

#...
tests/algorithms/sbi/test_mcabc.py:26: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
sbibm/algorithms/sbi/mcabc.py:113: in run
    log_prob_true_parameters = kde_posterior.log_prob(true_parameters.squeeze())
sbibm-main-venv/lib64/python3.9/site-packages/sbi/utils/kde.py:31: in log_prob
    self.kde.score_samples(parameters_unconstrained.numpy()).astype(np.float32)
sbibm-main-venv/lib64/python3.9/site-packages/sklearn/neighbors/_kde.py:231: in score_samples
    X = self._validate_data(X, order="C", dtype=DTYPE, reset=False)
sbibm-main-venv/lib64/python3.9/site-packages/sklearn/base.py:561: in _validate_data
    X = check_array(X, **check_params)

#...
                # If input is 1D raise error
                if array.ndim == 1:
>                   raise ValueError(
                        "Expected 2D array, got 1D array instead:\narray={}.\n"
                        "Reshape your data either using array.reshape(-1, 1) if "
                        "your data has a single feature or array.reshape(1, -1) "
                        "if it contains a single sample.".format(array)
                    )
E                   ValueError: Expected 2D array, got 1D array instead:
E                   array=[ 0.73016036 -0.3217839 ].
E                   Reshape your data either using array.reshape(-1, 1) if your data has a single feature or array.reshape(1, -1) if it contains a single sample.

see 74d92a3 (#30)

janfb commented 2 years ago

regarding the error: this is a transform bug in sbi. will fix it and make a new release. thanks for catching this, yet another point for making these unit tests!

psteinb commented 2 years ago

Ok, then let's wait for sbi to fix that.

janfb commented 2 years ago

This is fixed now in sbi. Here, the .squeeze() of the true_parameters needs to be removed for this to work: the log_prob methods wants batched parameters, as common in torch. So this needs to be fixed both in mcabc and smcabc.

janfb commented 2 years ago

the pin to sbi 0.17.2. is merged into main already, so if you rebase on it and reinstall it should all be up to date.

jan-matthis commented 2 years ago

Here, the .squeeze() of the true_parameters needs to be removed for this to work: the log_prob methods wants batched parameters, as common in torch. So this needs to be fixed both in mcabc and smcabc.

These are the respective lines:

Would be great if you could make this part of the PR!

psteinb commented 2 years ago

I discovered some minor things not working in smcabc. Should be ready for review with 4d129e0 (#30)

jan-matthis commented 2 years ago

Tests are passing now 🚀