Corrected a bug on the rejection_sampling_2D algorithm and updated th…

[mhurte]

Hi everyone,

I would like to share a fix for a bug that I found as I was testing some sampling features of this library.

As I tried to compute the acceptance rate for the rejection sampling algorithm in 2D that is implemented in the sampling.py file, I noticed that something was odd : The following figure shows the acceptance rate as a function of sigma before the fix, compaired to the theoretical one for this method.

Since we know what the theoretical acceptance rate is for this algorithm, it was easy to notice that something was wrong when plotting the figure above. The acceptance rate is quickly converging to 1 when it should be converging to zero.

After searching through the code what the matter could be, I eventually found out that the roles of the mu_a and mu_b as defined in the functions _rejection_sampling_2D_gfunction_plus, _rejection_sampling_2D_gfunction_minus and _rejection_sampling_2D were in fact inverted.

After fixing this, the program now behaves properly, the following figure shows the acceptance rate as a function of sigma after the fix.

Moreover, I added a boolean parameter to _rejection_sampling_2D, return_rate (defaulted to False) that allows to get the acceptance rate for the generation along with the sample if necessary.
I also used the _pdf_r function that was already implemented in the file in the functions _rejection_sampling_2D_gfunction_minus and _rejection_sampling_2D_gfunction_plus to make the code more readable.

The documentation has been updated accordingly.

[plcrodrigues]

I think that calling it return_acceptance_rate would be better, since one could think that it is the rejection_rate or any other bizarre rate.

[mhurte]

Indeed, I've changed it.

[plcrodrigues]

Thanks @mhurte! This is very helpful.

[qbarthelemy]

Thx @mhurte for this contribution!
Can you complete whatsnew file, and complete tests with the new parameter?

[mhurte]

Hi,
I am having a couple of issues with my pullrequest :
For some reason I am unable to run the pytest locally before pushing my changes as it leads to the following error

ImportError while loading conftest 'C:\Users\Eldor\Desktop\Commit\pyRiemann\tests\conftest.py'.
tests\conftest.py:5: in <module>
    from pyriemann.datasets import make_matrices, make_masks
E   ImportError: cannot import name 'make_matrices' from 'pyriemann.datasets' (C:\Users\Eldor\AppData\Local\Programs\Python\Python311\Lib\site-packages\pyriemann\datasets\__init__.py)

@plcrodrigues seems to be having the same issue as me

Also, during the lasts tests that automatically ran on github I noticed that the test that is not working is the following :

=================================== FAILURES ===================================
____________________________ test_tlrotate[euclid] _____________________________

rndstate = RandomState(MT19937) at 0x7F0C69F626B0, metric = 'euclid'

    @pytest.mark.parametrize("metric", ["euclid", "riemann"])
    def test_tlrotate(rndstate, metric):
        """Test pipeline for rotating the datasets"""
        # check if the distance between the classes of each domain is reduced
        X, y_enc = make_classification_transfer(
            n_matrices=25, class_sep=5, class_disp=1.0, random_state=rndstate)
        rct = TLCenter(target_domain='target_domain')
        X_rct = rct.fit_transform(X, y_enc)
        rot = TLRotate(target_domain='target_domain', metric=metric)
        X_rot = rot.fit_transform(X_rct, y_enc)
        _, y, domain = decode_domains(X_rot, y_enc)
        for label in np.unique(y):
            d = 'source_domain'
            M_rct_label_source = mean_riemann(
                X_rct[domain == d][y[domain == d] == label])
            M_rot_label_source = mean_riemann(
                X_rot[domain == d][y[domain == d] == label])
            d = 'target_domain'
            M_rct_label_target = mean_riemann(
                X_rct[domain == d][y[domain == d] == label])
            M_rot_label_target = mean_riemann(
                X_rot[domain == d][y[domain == d] == label])
            d_rct = distance_riemann(M_rct_label_source, M_rct_label_target)
            d_rot = distance_riemann(M_rot_label_source, M_rot_label_target)
>           assert d_rot <= d_rct
E           assert 0.12430437458015[68](https://github.com/mhurte/pyRiemann/actions/runs/5312005943/jobs/9615952189#step:7:69)5 <= 0.124246099[81](https://github.com/mhurte/pyRiemann/actions/runs/5312005943/jobs/9615952189#step:7:82)3[95](https://github.com/mhurte/pyRiemann/actions/runs/5312005943/jobs/9615952189#step:7:96)297

tests/test_transfer.py:121: AssertionError

And it does not seem to be related to the files that I have modified so far.
Any idea where the problem could come from ?
Thank you in advance.

[plcrodrigues]

In fact, it seems that to run pytest locally one has to do:

python -m pytest tests/

Source: https://stackoverflow.com/a/34140498/1898001

[plcrodrigues]

The changes that you made in _rejection_sampling_2D had a direct impact on the sampling functions that generate a test dataset for the unit test test_tlrotate.

I think that the hyper-parameters that we had chosen for creating the dataset were not really good, so I changed them so to have an example where the distance between the class means after rotation was indeed smaller than after just re-center.

[qbarthelemy]

Thx @mhurte !