Skip to content

Fix docstring MDTopology to MDTrajTopology#1010

Merged
dwhswenson merged 3 commits into
openpathsampling:masterfrom
sroet:fix_docstring
Apr 15, 2021
Merged

Fix docstring MDTopology to MDTrajTopology#1010
dwhswenson merged 3 commits into
openpathsampling:masterfrom
sroet:fix_docstring

Conversation

@sroet

@sroet sroet commented Apr 15, 2021

Copy link
Copy Markdown
Member

A student asked my help initializing the OpenMMEngine and I could not find the openpathsampling.engines.openmm.MDTopology. Looking at openpathsampling/engines/openmm/topology.py this is actually called: MDTrajTopology

This PR all the docstrings that mention MDTopology.
it also fixes some of the trivial pep8 complaints in openmm/engine.py

@sroet sroet changed the title Fix docstring Fix docstring MDTopology to MDTrajTopology Apr 15, 2021
@codecov

codecov Bot commented Apr 15, 2021

Copy link
Copy Markdown

Codecov Report

Merging #1010 (82c0a55) into master (55716a0) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1010      +/-   ##
==========================================
- Coverage   81.19%   81.19%   -0.01%     
==========================================
  Files         139      139              
  Lines       15130    15129       -1     
==========================================
- Hits        12285    12284       -1     
  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% <ø> (-0.19%) ⬇️

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 55716a0...82c0a55. Read the comment docs.

@sroet

sroet commented Apr 15, 2021

Copy link
Copy Markdown
Member Author

The -0.19% coverage in openmm/engine.py is the removal of the unneeded import of copy (while the number of not-coverd lines stays the same, so a lower percentage is covered)

@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.

Thanks for the cleanup. Looks good except one tiny request to undo one change.

General thought on this: I think that MDTrajTopology should be moved out of OpenMM and into engines.topology (with canonical import as paths.engines.MDTrajTopology). I also sometimes have to go hunting for it, and the point is that it isn't an OpenMM-specific tool. It's used (implicitly) in the Gromacs engine, and will be relevant for most other MD engines anyway. (Obviously such a change would come with continued support -- with deprecation -- at the current locations where it can be imported.)

Comment thread openpathsampling/collectivevariables/plumed_wrapper.py Outdated
Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>
@dwhswenson dwhswenson merged commit cb22b34 into openpathsampling:master Apr 15, 2021
@sroet sroet deleted the fix_docstring branch April 15, 2021 14:10
@dwhswenson dwhswenson added docs issues/PRs related to documentation misc PR labels Apr 25, 2021
@dwhswenson dwhswenson mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs issues/PRs related to documentation misc PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants