Skip to content

add test files#2940

Merged
IAlibay merged 3 commits intoMDAnalysis:developfrom
lilyminium:add-test-files
Sep 9, 2020
Merged

add test files#2940
IAlibay merged 3 commits intoMDAnalysis:developfrom
lilyminium:add-test-files

Conversation

@lilyminium
Copy link
Member

Fixes #2939

Changes made in this Pull Request:

  • add test file patterns in setup.py

I think this is why they're not included in the release. Maybe we should add a note to datafiles.py?

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #2940 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2940      +/-   ##
===========================================
- Coverage    93.01%   93.00%   -0.02%     
===========================================
  Files          187      187              
  Lines        24940    24895      -45     
  Branches      3261     3261              
===========================================
- Hits         23198    23153      -45     
  Misses        1694     1694              
  Partials        48       48              
Impacted Files Coverage Δ
auxiliary/base.py 90.75% <0.00%> (-0.57%) ⬇️
coordinates/base.py 94.85% <0.00%> (-0.27%) ⬇️
util.py 89.39% <0.00%> (-0.08%) ⬇️
transformations/__init__.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/fit.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/wrap.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/rotate.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/__init__.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/translate.py 100.00% <0.00%> (ø)
...ge/MDAnalysis/transformations/positionaveraging.py 100.00% <0.00%> (ø)
... and 1 more

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 b2b7bcb...baa871f. Read the comment docs.

@orbeckst
Copy link
Member

orbeckst commented Sep 9, 2020

Thank you. Did you manually try building and running the failing tests?

@lilyminium lilyminium force-pushed the add-test-files branch 2 times, most recently from a615aaf to e835efa Compare September 9, 2020 05:29

@pytest.fixture(scope='module')
def pca_aligned(u):
u.transfer_to_memory()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is most efficiently done (and avoids numerous other "transfer_to_memory() calls) if u is loaded in_memory=True to start off with but not sure if there's a reason for this sequential nature of events

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, in the original issue I couldn't get the code to work even if the test was isolated. It's possible I made a mistake somewhere, but definitely would be worth testing in isolation.

@lilyminium
Copy link
Member Author

@orbeckst yes, I built the package from a new clone python setup.py sdist and ran tests on that. pytest -n 12 had no errors. python setup.py sdist on the directory I normally work out of includes all test files in the distribution regardless of what's in package_data for some reason. That might be how it slipped detection in the pre-release tests.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side this looks fine. Thank you @lilyminium

@IAlibay if you have more comments on the transfer_to_memory() then please block the PR.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll temporarily block this to allow for clarification (it's possible I'm just missing something really obvious here).

So the transfer to memory does somehow fix test_given_mean, but it's not exactly clear to me how/why pca_aligned._mean changes on memory transfer.

In fact, in both cases pca._mean is set to a NoneType? Unless I'm missing something obvious, it looks like transferring to memory just ends up providing a pre-aligned trajectory to test_given_mean which renders pca._mean obsolete?

I'm really hoping that I'm missing something obvious, but it looks to me like in all cases the else of this branch is never reached?

if self._mean is None:
self.mean = np.zeros(self._n_atoms*3)
self._calc_mean = True
else:
self.mean = self._mean.positions
self._calc_mean = False

This would explain why codecov has it as untested? https://codecov.io/gh/MDAnalysis/mdanalysis/src/develop/package/MDAnalysis/analysis/pca.py

Edit:

Obviously this entire question is based on the fact that I assume that test_given_mean should be testing what happens if mean is given and self._calc_mean is True. If that's not the intended purpose then that's fine.

@lilyminium
Copy link
Member Author

As @IAlibay noted the test is a bit meaningless so I've reverted that for now and can fix in a separate PR.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@IAlibay
Copy link
Member

IAlibay commented Sep 9, 2020

How odd, one of the travis jobs ran for over 50 mins, I've restarted it to see if it's just a bad node.

@IAlibay IAlibay merged commit 3148207 into MDAnalysis:develop Sep 9, 2020
@IAlibay
Copy link
Member

IAlibay commented Sep 9, 2020

Thanks for the fix @lilyminium. Note; we'll probably also need to push this to master for 1.0.1.

@orbeckst
Copy link
Member

I'll do the cherry-pick to master.

orbeckst pushed a commit that referenced this pull request Sep 10, 2020
* Fixes issue #2939

* Work done in this PR
  - Adds missing file types of testsuite/setup.py
    (*top, *in, *sdf, and the contents of data/gromacs/gromos54a7_edited.ff/*).

(cherry picked from commit 3148207)
@orbeckst
Copy link
Member

Fixed in 2ebb052 on master.

PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
Fixes issue MDAnalysis#2939 

## Work done in this PR
- Adds missing file types of testsuite/setup.py
  (*top, *in, *sdf, and the contents of data/gromacs/gromos54a7_edited.ff/*).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

installed MDAnalysisTests error/fail due to missing files

4 participants