Skip to content

Move MDTrajTopology#1012

Merged
dwhswenson merged 9 commits into
openpathsampling:masterfrom
sroet:move_mdtrajtopology
Apr 19, 2021
Merged

Move MDTrajTopology#1012
dwhswenson merged 9 commits into
openpathsampling:masterfrom
sroet:move_mdtrajtopology

Conversation

@sroet

@sroet sroet commented Apr 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

closes #1011

This PR moves MDTrajTopology from openpathsampling.engines.openmm.topology to openpathsampling.engines.topology

This was a slightly bigger hassle than intended (because we want to warn on import).

I ended up mimicking importing ABCs from collections instead of collections.abc is deprecated

This also makes mdtraj a hard requirement

sroet
sroet commented Apr 15, 2021
View reviewed changes
Comment thread openpathsampling/engines/openmm/topology.py
sroet
sroet commented Apr 15, 2021
View reviewed changes
Comment thread devtools/minimal.txt Outdated
sroet
sroet commented Apr 15, 2021
View reviewed changes
Comment thread setup.cfg
@codecov

codecov Bot commented Apr 15, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1012 (6b4bd08) into master (cb22b34) will increase coverage by 0.00%.
The diff coverage is 60.93%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1012   +/-   ##
=======================================
  Coverage   81.19%   81.20%           
=======================================
  Files         139      138    -1     
  Lines       15129    15134    +5     
=======================================
+ Hits        12284    12289    +5     
  Misses       2845     2845
Impacted Files Coverage Δ
openpathsampling/collectivevariable.py 66.88% <ø> (ø)
...pathsampling/collectivevariables/plumed_wrapper.py 100.00% <ø> (ø)
openpathsampling/engines/openmm/engine.py 70.00% <ø> (ø)
openpathsampling/engines/topology.py 61.53% <57.62%> (-38.47%) ⬇️
openpathsampling/deprecations.py 100.00% <100.00%> (ø)
openpathsampling/engines/__init__.py 100.00% <100.00%> (ø)
openpathsampling/engines/gromacs/engine.py 95.65% <100.00%> (ø)
openpathsampling/engines/openmm/tools.py 89.11% <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 cb22b34...6b4bd08. Read the comment docs.

@dwhswenson

dwhswenson commented Apr 17, 2021

Copy link
Copy Markdown
Member

Note (as much to self as to others): This can be merged despite the CodeCov fails. The changes are moving code that was not fully covered. Therefore the patch code isn't covered.

I suspect that the drop in project coverage (7 lines) may be due to making MDTraj a hard requirement. (It shouldn't be, see #1012 (comment) for suggested fix.)

@sroet sroet requested a review from dwhswenson April 18, 2021 16:09
@sroet

sroet commented Apr 18, 2021

Copy link
Copy Markdown
Member Author

@dwhswenson, reverted the hard mdtraj dependency. Please have another look (even though codecov is still not happy about it).

dwhswenson
dwhswenson requested changes Apr 19, 2021
View reviewed changes

@dwhswenson dwhswenson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks basically good. Only changes:

  1. deprecated_in should be next release (1.5.0)
  2. Please import MDTrajTopology into the openpathsampling.engines namespace (via the __init__.py), just as Topology is. On this point, I do care that the deprecation message tells people to load from openpathsampling.engines, since that will be the canonical API location. However, I don't care as much about the docstrings. Besides, I think the more correct way to write the docstrings would be :class:`.MDTrajTopology`.

Comment thread openpathsampling/deprecations.py Outdated
Comment thread openpathsampling/deprecations.py Outdated
sroet and others added 2 commits April 19, 2021 16:04
@sroet @dwhswenson
Apply suggestions from code review
8497713 
Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>
@sroet
allow import from paths.engines
6b4bd08
@dwhswenson

dwhswenson commented Apr 19, 2021

Copy link
Copy Markdown
Member

Looks good now, thanks! Merging.

@dwhswenson dwhswenson merged commit 44238b2 into openpathsampling:master Apr 19, 2021
@sroet sroet deleted the move_mdtrajtopology branch April 20, 2021 11:14
@dwhswenson dwhswenson mentioned this pull request Apr 20, 2021
Merged
@dwhswenson dwhswenson mentioned this pull request Jul 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dwhswenson dwhswenson dwhswenson approved these changes

Assignees

No one assigned

Labels

Deprecations misc PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MDTrajTopology should be moved from engines.OpenMM to engines.topology

2 participants

@sroet @dwhswenson