Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses failure of test_load() in PSA (#2049) #2239

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

sseyler
Copy link
Contributor

@sseyler sseyler commented Apr 8, 2019

Fixes #2049 and also boosts PSA code coverage (#1006).

Changes made in this Pull Request:

  • test_load() now passes after calling save_paths() with default arguments
  • PSAnalysis now has default base filename and prefix for consistency with default saving behavior of PSAnalysis.generate_paths()
  • Better consistency of Path.fit_to_reference() with MDAnalysis.analysis.AlignTraj
  • Better consistency of PSAnalysis.generate_paths() with MDAnalysis.analysis.AlignTraj
  • Updated PSA docs for consistency with above changes

PR Checklist

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

NOTE: load/save fails --- maybe a bug??
@sseyler sseyler changed the title Addresses PSA code coverage (#1006) and test_load() in PSA (#2049) Addresses failure of test_load() in PSA (#2049) Apr 8, 2019
@sseyler sseyler mentioned this pull request Apr 8, 2019
4 tasks
@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #2239 into develop will increase coverage by 0.07%.
The diff coverage is 62.5%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2239      +/-   ##
===========================================
+ Coverage     89.6%   89.67%   +0.07%     
===========================================
  Files          159      159              
  Lines        19728    19728              
  Branches      2780     2780              
===========================================
+ Hits         17677    17692      +15     
+ Misses        1457     1440      -17     
- Partials       594      596       +2
Impacted Files Coverage Δ
package/MDAnalysis/analysis/psa.py 83.49% <62.5%> (+2.87%) ⬆️

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 f878cc1...b49a6b0. Read the comment docs.

@orbeckst
Copy link
Member

orbeckst commented Apr 8, 2019

Thanks @sseyler , I'll wait for CI to finish. Feel free to ping me.

@orbeckst orbeckst self-assigned this Apr 8, 2019
@orbeckst
Copy link
Member

orbeckst commented Apr 8, 2019

Weird travis failure on macOS 960.10 (Python 2.7) and 960.9 (Python 3.6)

$ eval $MAIN_CMD $SETUP_CMD
/Users/travis/.travis/functions: line 104: pytest: command not found
The command "eval $MAIN_CMD $SETUP_CMD" exited with 127.

even though the installation logs indicate that pytest package was installed:

   pytest-4.4.0               |           py36_1         344 KB  conda-forge

@MDAnalysis/coredevs any idea what's wrong here?

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.

This passed CI on Linux and Windows but macOS has a weird failure not related to this PR so I am approving it for right now. Thanks @sseyler !

@tylerjereddy
Copy link
Member

I pushed this feature branch to my fork and opened a PR to the same master branch on my fork & so far the Mac job there is doing just fine with pytest running the suite: tylerjereddy#2

Maybe a restart is warranted, and if it starts showing up more regularly we can open an issue.

@tylerjereddy
Copy link
Member

Commit history isn't the cleanest, but CI is all green now if that's what you were waiting for.

@orbeckst
Copy link
Member

Many thanks @tylerjereddy . I'll quickly compact the history a bit.

@tylerjereddy
Copy link
Member

We're getting the MacOS failures again--have those happened in any PR other than this one?

@orbeckst
Copy link
Member

yes:

@orbeckst
Copy link
Member

I have to resolve the merge conflict in this PR so CI will run again anyway...

@orbeckst
Copy link
Member

Now AppVeyor doesn't want to run... will rebase manually.

@orbeckst
Copy link
Member

macOS still failing

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.

3 participants