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

RMSD, RMSF Refactor/Test Coverage #893

Closed
jdetle opened this issue Jul 7, 2016 · 6 comments
Closed

RMSD, RMSF Refactor/Test Coverage #893

jdetle opened this issue Jul 7, 2016 · 6 comments
Assignees
Milestone

Comments

@jdetle
Copy link
Contributor

jdetle commented Jul 7, 2016

Expected behaviour

RMSD and RMSF are highly used analysis modules. Both should follow the Bauhaus API and both should have full coverage.

Actual behaviour

MDAnalysis.analysis.rms.RMSD has no current test coverage, (which didn't expose the bug fixed by #889). (EDIT: fixed with PR #889) Both MDAnalysis.analysis.rms.RMSD and MDAnalysis.analysis.rms.RMSF are great candidates for a refactor to the Bauhaus API as well.

Proposal

Refactor MDAnalysis.analysis.rms.RMSD and MDAnalysis.analysis.rms.RMSF to conform to the Bauhaus API. As a part of this refactor we would add test coverage for RMSD. Additional work that could be done, but should probably be its own issue is creating the option for linear programming rmsd similar to that of MDTraj.

@orbeckst
Copy link
Member

I generally agree to the refactor, Bauhaus-style. (And many thanks for PR #889 which added tests for RMSD).

For enhancements I would open new issues/PRs. I would also like to automate RMSD-calculation between two non-identical structures. PyMOL (and LSQMAN) have algorithms that do RMSD iteratively and throw out atoms that have high RMSD. The analysis.align module already has some code to create a sequence alignment (align.sequence_alignment() using biopython Bio.pairwise2) and use that one. Doing all of these things automatically would be useful – but all of this is better broken into smaller issues.

@sseyler
Copy link
Contributor

sseyler commented Jul 19, 2016

Some time ago when the MDAnalysis.analysis.rms.RMSF class was added, @dotsdl mentioned in Issue #305 (referring also to #288) that certain useful features could be added in the future (e.g., on-the-fly fitting, saving data).

It might make more sense to open another issue for an RMSF (possibly also RMSD) functionality upgrade, but I wanted to note here that enabling on-the-fly fitting (in particular) would:

  1. Be relatively straightforward.
  2. Avoid the need to generate a new fitted trajectory for each unique atom selection prior to running RMSF.run(); I think this is particularly important as it saves (possibly major) headaches, especially when filesystem space is limited and one wants to avoid littering directories with otherwise identical trajectories differing only by fitting/orientation.
  3. Also give (greater) commonality in features between RMSD and RMSF.

Note: much of the code in MDAnalysis.analysis.rms.RMSD.run() could then be pulled out as a separate function in the module or possibly implemented as a method in a base class for (any) rmsd-type analysis classes.

I would be happy to work on this as a separate issue and leave the Bauhaus-style refactoring here.

@jbarnoud
Copy link
Contributor

For the record, the proposal about on-the-fly coordinate transformation is #786. I've been lacking time lately to work on it, but I can arrange some time if there is interest for the feature.

@sseyler
Copy link
Contributor

sseyler commented Jul 19, 2016

@jbarnoud Ah, excellent! Not sure why I missed that.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 20, 2016

@sseyler @jbarnoud is there anything stopping me from working on the refactor?

@jbarnoud
Copy link
Contributor

Please, go for the refactor. If anything, try to have as much of the logic as possible decoupled from the analyses class so it can be reused elsewhere.

@orbeckst orbeckst changed the title [Proposal] RMSD, RMSF Refactor/Test Coverage RMSD, RMSF Refactor/Test Coverage Jul 26, 2016
@orbeckst orbeckst removed the proposal label Jul 26, 2016
@jdetle jdetle mentioned this issue Aug 13, 2016
4 tasks
@orbeckst orbeckst added this to the 0.16.0 milestone Sep 23, 2016
orbeckst pushed a commit that referenced this issue Sep 23, 2016
* fixes #893 
* RMSD refactored with AnalysisBase class and confirms to the new analysis API (see #719)
* Tests added
* Documentation and chaining functions
* Fixed and updated documentation
* Addressed broadcasting and other issues
* Updated CHANGELOG
abiedermann pushed a commit to abiedermann/mdanalysis that referenced this issue Jan 5, 2017
* fixes MDAnalysis#893 
* RMSD refactored with AnalysisBase class and confirms to the new analysis API (see MDAnalysis#719)
* Tests added
* Documentation and chaining functions
* Fixed and updated documentation
* Addressed broadcasting and other issues
* Updated CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants