Implement the decorrelator

[xiki-tempula]

As is discussed in #159. I have implemented the decorrelators using the logic of the alchemical analysis.

[codecov]

Codecov Report

Merging #163 (e93472b) into master (093ad87) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   97.72%   97.78%   +0.06%     
==========================================
  Files          20       20              
  Lines        1097     1128      +31     
  Branches      230      236       +6     
==========================================
+ Hits         1072     1103      +31     
  Misses          5        5              
  Partials       20       20              

Impacted Files	Coverage Δ
src/alchemlyb/preprocessing/__init__.py	100.00% <100.00%> (ø)
src/alchemlyb/preprocessing/subsampling.py	100.00% <100.00%> (ø)

---

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 093ad87...e93472b. Read the comment docs.

[orbeckst]

Can you please explain what the three methods are? Neither the user nor I really knows what the code is supposed to be doing. Once I know what's supposedt o happen, I can review the code in a more meaningful manner.

The other comments are all pretty minor.

Please start a new section in CHANGES for 0.6.0 and add an entry.

Thanks for adding the functionality!

[xiki-tempula]

@orbeckst Thank you very much for the review. I have fixed everything else but is struggling with the pytest.fixture

alchemlyb/src/alchemlyb/tests/test_preprocessing.py

Line 191 in 093ad87

@pytest.mark.parametrize('series', [

As you can see, the old tests don't use the fixture. Normally, I use fixture or mark.parametrize but I realised now that there might be a problem with using both of them

@pytest.fixture()
def data():
    return [1, 2]

@pytest.mark.parametrize(('data', 'value'), [(data, 2), (data, 2)])
def test_value(data, value):
    assert len(data) == value

will not work, where it will complain that the pytest fixture is a function and has no len().

This seems to be a long-standing problem dating back to 2013 pytest-dev/pytest#349 and it seems that one could

• use the ugly solution suggested in the issue.
• don't use the fixture
• don't use parameterize

I wonder what is your feeling about this? I think abandoning the fixture might be the easiest way out.

[orbeckst]

For the fixture, I meant something along the following lines

@pytest.fixture
def dataset():
   return data.load_ABFE()

@pytest.make.parametrize("value,ref", ...)
def test_something(dataset, value, ref):
   ...

i.e., just not calling data.load_* inside but providing the data as a fixture. I'd think that this won't collide.

[xiki-tempula]

@orbeckst Thanks for the advice.
There is an existing test

alchemlyb/src/alchemlyb/tests/test_preprocessing.py

Line 191 in 093ad87

@pytest.mark.parametrize('series', [

which uses gmx_benzene_dHdl as a function, and I would need gmx_benzene_dHdl to be a fixture for my test.
I wonder what is your advice for transforming this test

alchemlyb/src/alchemlyb/tests/test_preprocessing.py

Line 192 in 093ad87

gmx_benzene_dHdl()['fep'][:20], # wrong length

? Thank you

[orbeckst]

Perhaps something like

@pytest.fixture(scope="class")   # class scope only if the data are never modified
def dataset(self):
   return gmx_benzene_dHdl()

@pytest.mark.paramtrize("stop,step", [(20, None), (None, -1)])
def test_series(self, dataset, start, stop):
   series = dataset[:stop:step]
   with pytest.raises(ValueError):
         self.slicer(dataset, series=series)

The advantage would be that with a class-scoped fixture, gmx_benzene_dHdl() is only loaded once.

[xiki-tempula]

@orbeckst Thank you very much for the advice, I do understand the advantage of fixtures and it is just that I don't know how to make it work with parameterized. There is another existing test that I'm not sure of how to make it into a fixture.

alchemlyb/src/alchemlyb/tests/test_preprocessing.py

Line 50 in 093ad87

@pytest.mark.parametrize(('data', 'size'), [(gmx_benzene_dHdl(), 661),

    @pytest.mark.parametrize(('data', 'size'), [(gmx_benzene_dHdl, 661),
                                                (gmx_benzene_u_nk, 661)])
    def test_basic_slicing(self, data, size):
        assert len(self.slicer(data(), lower=1000, upper=34000, step=5)) == size

Where gmx_benzene_dHdl and gmx_benzene_u_nk used to be functions and making them into fixture will not work in this case. Since there are only two cases here, I could just make it into two explicit tests, but I'm wondering if there could be a better way.

[orbeckst]

I think I would just leave this one.

There's a way to parametrize fixtures but it's not cleaner than what you already have for this test. If we don't re-use a fixture and it's not going to be easier to read then leave it, I'd say.

DRACO DORMIENS NUMQUAM TITILLANDUS

[xiki-tempula]

@orbeckst Thanks for the advice. I have eventually left the old test untouched and have only applied the fixture to the new tests.

[orbeckst]

lgtm, will just add my minor fixes