alchemistry / alchemlyb

the simple alchemistry library
https://alchemlyb.readthedocs.io
BSD 3-Clause "New" or "Revised" License
191 stars 49 forks source link

Clean up the iloc in the tests #254

Closed xiki-tempula closed 1 year ago

xiki-tempula commented 1 year ago

Fix #202

I have cleaned up most of the iloc in the test but there are a couple of tests that still use iloc. Most of them are parameterised tests where different datasets demand different column labels.

    @pytest.mark.parametrize(('data', 'size'), [(gmx_benzene_dHdl(), 4001),
                                                (gmx_benzene_u_nk(), 4001)])
    def test_subsampling(self, data, size):
        assert len(self.slicer(data, series=data.iloc[:, 0])) <= size

In this case, the first column is always chosen which have different column name for different datasets.

Another case is

    def get_delta_f(self, est):
        ee = 0.0

        for i in range(len(est.d_delta_f_) - 1):
            ee += est.d_delta_f_.values[i][i+1]**2
        return est.delta_f_.iloc[0, -1], ee**0.5

Where depending on the dataset, this could be est.delta_f_[0.0][1.0] or est.delta_f_.loc[(0.0,0.0)][(1.0,1.0)]

codecov[bot] commented 1 year ago

Codecov Report

Merging #254 (45c9e0a) into master (079a5b5) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #254   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          26       26           
  Lines        1761     1761           
  Branches      379      379           
=======================================
  Hits         1738     1738           
  Misses          3        3           
  Partials       20       20           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

orbeckst commented 1 year ago

I think the remaining iloc cases are fine. Could you add a short comment stating why these are ilocs, just so that what you found now is not forgotten?

orbeckst commented 1 year ago

PLease resolve the conflicts and ping me when you need a review.

xiki-tempula commented 1 year ago

@orbeckst I have merged the master into this branch.

xiki-tempula commented 1 year ago

@orbeckst For the PEP8, I guess I could just do a project-wide black, which is the same as the one for flamel?

orbeckst commented 1 year ago

Can we do black in a separate PR? Reformatting hides any relevant changes. For this PR, do the few fixes manually and then we can blackify in a separate PR.

xiki-tempula commented 1 year ago

@orbeckst Would it be easier if I merge this PR as it is. Then do a black PR? For the .loc[0][0] related things, I cannot change them. For the space after the ,, black will automatically sort them out.