Skip to content

Added unwrap/wrap transformations#2038

Merged
orbeckst merged 2 commits intoMDAnalysis:developfrom
davidercruz:wrap-unwrap
Nov 18, 2019
Merged

Added unwrap/wrap transformations#2038
orbeckst merged 2 commits intoMDAnalysis:developfrom
davidercruz:wrap-unwrap

Conversation

@davidercruz
Copy link
Contributor

Adds a wrap transformation based on atoms.wrap and a unwrap transformation based on @richardjgowers 's _cutil.make_whole
Replaces PR #2020

PR Checklist

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

@jbarnoud
Copy link
Contributor

There may have been a solution to save the previous PR, but this is much easier.

2 comments:

Except for that, it looks good to me.

@codecov
Copy link

codecov bot commented Aug 11, 2018

Codecov Report

Merging #2038 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2038      +/-   ##
===========================================
+ Coverage    89.98%   89.99%   +<.01%     
===========================================
  Files          176      177       +1     
  Lines        22055    22075      +20     
  Branches      2896     2897       +1     
===========================================
+ Hits         19846    19866      +20     
  Misses        1612     1612              
  Partials       597      597
Impacted Files Coverage Δ
package/MDAnalysis/transformations/wrap.py 100% <100%> (ø)
package/MDAnalysis/transformations/__init__.py 100% <100%> (ø) ⬆️
transformations/__init__.py 100% <0%> (ø) ⬆️

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 e110108...05ef9d1. Read the comment docs.

@orbeckst
Copy link
Member

If there are any more changes on this branch you will need to merge or rebase against develop because #2066 was "fixed".

@orbeckst
Copy link
Member

@jbarnoud can you please make your comments #2038 (comment) a formal review so that the PR gets a chance to get merged?

@orbeckst
Copy link
Member

@davidercruz (and @jbarnoud ?) could you please summarize what needs to be done to get this PR done?

I am not looking for a promise that this gets done over the next weekend/holidays but something that will help to get the work done. This is an open source project so I understand that everyone is short on time and priorities shift but it would be a real shame if this didn't make it into MDAnalysis. It is always possible that someone else might find the energy to finish it (e.g., @ianmkenney in my group has recently been playing with the OTFT) but in this case one should make it easy for others to pick up the work, especially if one has not been able to finish things.

@richardjgowers
Copy link
Member

richardjgowers commented Dec 16, 2018 via email

@davidercruz
Copy link
Contributor Author

davidercruz commented Dec 21, 2018

Hi all, the PR is failing due to taking too long on travis. It's probably caused by the tests that use the GRO and TPR systems used in the test_wrap_with_compounds in the test module. This function tests the compound flag for the wrap transformation, and thus requires a test system where the flags group, residues, segments and fragments are applicable.

The transformation themselves are very simple, as they act as a caller for the wrap and unwrap functions of the Atomgroup. I just need someone to review the code and I'll make the changes during the holidays.

Merry Christmas

@zemanj zemanj mentioned this pull request Jan 30, 2019
4 tasks
@orbeckst
Copy link
Member

@jbarnoud @richardjgowers @davidercruz any chance that this gets into 0.20.0 #2240 (as we had promised in the 0.19.x blog post)?

orbeckst added a commit to MDAnalysis/binder-notebook that referenced this pull request Apr 26, 2019
- fix #13
- needs MDAnalysisData >= 0.7.0 for the "membrane peptide" dataset
- needs MDAnalysis >= 0.20.0 for the transformations
  (currently PRs MDAnalysis/mdanalysis#2038 ,
  MDAnalysis/mdanalysis#1991 , and
  MDAnalysis/mdanalysis#1973 )
- updated text in notebook
- adde a few more empty lines for clarity
orbeckst added a commit to MDAnalysis/binder-notebook that referenced this pull request Apr 29, 2019
- fix #13
- needs MDAnalysisData >= 0.7.0 for the "membrane peptide" dataset
- needs MDAnalysis >= 0.20.0 for the transformations
  (currently PRs MDAnalysis/mdanalysis#2038 ,
  MDAnalysis/mdanalysis#1991 , and
  MDAnalysis/mdanalysis#1973 )
- updated text in notebook
- adde a few more empty lines for clarity
@orbeckst
Copy link
Member

I merged develop into this branch and updated the CHANGELOG entry (has to go into 0.21).

- simple wrap/unwrap transformations (needs fragments)
- compound tests are now done on multiple segment universes
- added docs
- updated changelog
  new feature wrap transformation in 0.21.0.
@orbeckst
Copy link
Member

I rewrote the history and rebased against develop.

Merge this branch (don't rebase).

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.

I give this a preliminary LGTM. I will approve once CI looks good.

@davidercruz @jbarnoud @richardjgowers and last minute reasons why this should not get merged for 0.21?

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.

All tests pass and the basics look sensible, so I give it an "let's run with it".

@orbeckst orbeckst merged commit 3b0bdce into MDAnalysis:develop Nov 18, 2019
@orbeckst
Copy link
Member

So – this is finally done! Thank you @davidercruz and @jbarnoud !

(Now it just needs to be fast... but that's a different story, starting with #2376.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants