Bond-Angle-Torsions coordinate module #2668

daveminh · 2020-04-22T20:07:09Z

Changes made in this Pull Request:

I've written a module that will convert coordinates between a Cartesian and a Bond-Angle-Torsion coordinate system.

PR Checklist

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

Wrote converter to BAT and back to Cartesian. Need to add tests.

codecov · 2020-04-22T21:46:18Z

Codecov Report

Merging #2668 into develop will increase coverage by 0.03%.
The diff coverage is 96.95%.

@@             Coverage Diff             @@
##           develop    #2668      +/-   ##
===========================================
+ Coverage    92.18%   92.22%   +0.03%     
===========================================
  Files          183      184       +1     
  Lines        23977    24141     +164     
  Branches      3090     3123      +33     
===========================================
+ Hits         22104    22263     +159     
- Misses        1808     1813       +5     
  Partials        65       65

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/bat.py	`96.95% <96.95%> (ø)`

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 3d95282...7431ea2. Read the comment docs.

richardjgowers

@daveminh this looks like a very cool contribution! Would it make sense for this to also be available as a on the fly transformation?

package/MDAnalysis/analysis/bat.py

testsuite/MDAnalysisTests/analysis/test_bat.py

package/MDAnalysis/analysis/bat.py

daveminh · 2020-04-23T12:25:52Z

@daveminh this looks like a very cool contribution! Would it make sense for this to also be available as a on the fly transformation?

Thanks for the useful feedback! I will work on addressing each point. This is my first contribution to the project (and actually any community-run open-source project), so I am unfamiliar with some of the practices.

daveminh · 2020-04-23T12:28:08Z

@daveminh this looks like a very cool contribution! Would it make sense for this to also be available as a on the fly transformation?

I'm open to the idea, but I don't think it makes sense as an on-the-fly transformation because it doesn't change the Cartesian coordinates of the system.

daveminh · 2020-04-23T17:38:49Z

The latest commit should resolve all issues except for inclusion in duecredit

orbeckst

Sorry that review has been taking a while, we were pretty busy with GSoC. I have primarily requests for improving the documentation, see inline comments. Additionally:

Your test coverage is still a bit low so you should check the coverage report to see which lines are not covered yet.
You checked in a duecredit file and some profraw files; they should be removed.
The documentation build failed (see #2238.6) because you still need to reference your bat.rst in the Structure section of package/doc/sphinx/source/documentation_pages/analysis_modules.rst.

package/MDAnalysis/analysis/bat.py

testsuite/MDAnalysisTests/analysis/test_bat.py

package/MDAnalysis/analysis/bat.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Syncing fork

Wrote converter to BAT and back to Cartesian. Need to add tests.

* Revised torsion search. * Can load and save results.

Added test for nc I/O

orbeckst

Thanks for addressing my earlier comments.

Make BAT.bat a numpy array unless there's a really good reason for a list of np arrays.
There are a number of API things where I would like your new class behave more like other classes.
We removed almost all load/save methods (see remove save() methods from analysis classes #1745 for details), but in this case it might be justified to keep them. But then load/save need some sanity checks.
Various doc things – see comments.

package/MDAnalysis/analysis/bat.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

orbeckst

Thanks. A few more comments inline – some minor formatting and one question regarding renaming of self.bat to perhaps self.bat_trajectory.

package/MDAnalysis/analysis/bat.py

orbeckst · 2020-06-18T21:58:17Z

package/MDAnalysis/analysis/bat.py

+            ag must have a bonds attribute. If not available,
+            bonds may be guessed using
+            :meth:`AtomGroup.guess_bonds <MDAnalysis.core.groups.AtomGroup.guess_bonds>`.
+            ag must only include one molecule.


Adding the mark-up makes it clear to users that you're referring to the argument from the docs.

package/MDAnalysis/analysis/bat.py

orbeckst · 2020-06-18T23:51:14Z

Looks good to me, @daveminh . I assume when you build the docs locally they look ok? If you want to reorganize your commit history in a few (<5) commits that stand on their own (e.g., feature, tests, CHANGELOG+AUTHORS) then please rewrite your history. Otherwise we will squash everything into a single commit and do a merge.

@richardjgowers any final comments?

orbeckst · 2020-06-18T23:59:26Z

Regarding OTF transformations (#2668 (review)) one could start thinking about something where you use BAT, modify internal coordinates on the fly, and then pass that as the actual coordinates. Similarly, one could provide BAT as an AUXReader to have the internal coordinates as a parallel trajectory. But that's all beyond the current PR.

Another future improvement might be to select the correct BAT for a given atom – this is currently really complicated because there isn't a 1-to-1 correspondence between the BAT 3N array and the atoms. Not sure what this "topology-like" data structure would be but it could mirror our standard topology.

EDIT: Maybe not really complicated, but more "requires more work than it should to figure out offsets in self.bat".

daveminh · 2020-06-19T00:08:40Z

I see no need to reorganize the history, so please go ahead and squash everything into one commit and do a merge.

Oliver and Richard, thanks for looking things over carefully several times! I've never had code reviewed so thoroughly before. I've also never tried many of the software engineering practices. It was tedious the first time but I'm sure with practice it'll be easier. In time, I hope to be able to develop code that's more broadly useful in the scientific community and better guide my group members to do the same.

Presently I'm not personally planning to pursue more work with BAT itself. Rather I am using it as a stepping stone to a new(ish) class of end-point free energy calculations. I'll probably put in a new pull request around the time I'm ready to submit a paper.

daveminh · 2020-06-24T12:14:33Z

I'm just checking in on this.

orbeckst · 2020-06-24T17:35:29Z

ping @richardjgowers

stale review, as far as I can tell @richarjgower's requests were addressed

orbeckst · 2020-07-01T07:52:01Z

@daveminh can you please fix the conflict in CHANGELOG?

I can't do it for you because I don't have permission to modify your branch. There's a way to enable this and then I could have done it myself.

Please ping me when the PR is mergeable and the CI is green. Thanks.

orbeckst

Sorry to be pedantic, but the CHANGELOG got slightly messed up. Please fix – many thanks.

I'll squash and merge afterwards. Thanks for your patience!

package/CHANGELOG

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

orbeckst · 2020-07-01T17:09:34Z

Thank you and congratulations for your first contribution to MDAnalysis, @daveminh ! I also sent you an invitation as a MDAnalysis/contributor.

BAT will be in 2.0.0. 🎉

If there are issues related to the module then we will almost certainly ping you and possibly ask you for comments or code reviews. It really helps when original code authors respond so that we can quickly address issues so please look out for @-mentions from MDAnalysis.

* new MDAnalysis.analysis.bat module: convert between internal bond-angle-torsion and Cartesian coordinates (based on published research) * standard analysis class * can load and save results (numpy format) * references with duecredit * tests * docs with example * update CHANGELOG and AUTHORS

@tbengtsen

Update of AUTHORS and CHANGELOG with inferred author contributions. * Removed duplicate mattwthompson in 0.20.0 changelog entry.: mattwthompson was placed twice by accident, this removes this duplication. * Addition of missing authors. An retrospective analysis of the authors found via `git shortlog -s -n --email --branches="develop"` found several commits by authors which were not present in the `AUTHORS.md` file. - Zhenbo Li: commited via PR: Started tests for gnm. #803 and Make Travis run tests on OSX. #771, - Jenna M. Swarthout via PR Update CoC according to suggestions from current CoC committee #4289 and Point to new reporting form link (owned by conduct@mdanalysis.org) #4298, - Bradley Dice via PR Fix GSD Windows compatibility #2384 , - David Minh via PR #2668 There seemed to be no indications in the above mentioned PRs that the author did not want to be in the authors file, it looks like it was just forgotten. * Addition of missing entries from the changelog Continued cross referencing of the git shortlog output but also accounting for the changelog identified several individuals that had not been included in the changelog entries for the release they contributed under. They were added to the relevant entry of the changelog based on the merge commit date. Please note that for Tone Bengsten, we a) had no github handle (so they were assigned @tbengtsen), and b) no specific commit. Given we know that this individual was including alongside the encore merge, they were assigned to the 0.16.0 release. * Update CHANGELOG * PR #1218 * PR #1284 and #1408 * PR #4109 * based on git history * PRs #803 and #771 (author addition, changelog addition) * PR #2255 and #2221 * PR #1225 * PR #4289 and #4298 * PR #4031 * PR #4085 * PR #3635 * PR #2356 * PR #2559 * No GH handle - Encore author addition @tbengtsen * PR #4184 * PR #2614 * PR #2219 * PR #2384 * PR #2668 * Add missing entry for Jenna Thanks to @fiona-naughton for helping out with digging into this data :) Co-authored-by: Fiona Naughton <fiona@mdanalysis.org> Co-authored-by: Oliver Beckstein <orbeckst@mdanalysis.org>

daveminh added 3 commits April 21, 2020 21:36

Create bat.py

cfd1630

Wrote converter to BAT and back to Cartesian. Need to add tests.

renamed variables as private, ran yapf, worked on documentation

7f5fc34

Added test

4438388

richardjgowers previously requested changes Apr 23, 2020

View reviewed changes

richardjgowers self-assigned this Apr 23, 2020

Resolved issues prior to pull request

95c4b9f

orbeckst requested changes May 15, 2020

View reviewed changes

daveminh and others added 15 commits June 15, 2020 14:21

Update package/MDAnalysis/analysis/bat.py

4464a32

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Merge remote-tracking branch 'upstream/develop' into develop

8355a27

Syncing fork

Create bat.py

6d527a8

Wrote converter to BAT and back to Cartesian. Need to add tests.

renamed variables as private, ran yapf, worked on documentation

044833e

Added test

86c4aa4

Resolved issues prior to pull request

38d5939

Updates to bat.py

e5a5a35

* Revised torsion search. * Can load and save results.

Integrated some suggestions from code review

4799176

Updated test and documentation

435a7a4

Merge branch 'BAT' into develop

2a2ecca

Adding bat back

af250d4

Delete .duecredit.p

8bf9134

Removing default.profaw

359902f

Added test for nc I/O

e0e8532

Merge pull request #2 from CCBatIIT/BAT

8203d78

Added test for nc I/O

orbeckst requested changes Jun 17, 2020

View reviewed changes

daveminh and others added 3 commits June 17, 2020 18:43

Update package/MDAnalysis/analysis/bat.py

115805c

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Update package/MDAnalysis/analysis/bat.py

f6c9cc7

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Updating documentation

62cd6eb

Update test_bat.py

ee5d05c

daveminh requested review from orbeckst and richardjgowers June 18, 2020 20:08

Delete test_bat_IO.npy

62bdaac

orbeckst requested changes Jun 18, 2020

View reviewed changes

Refining bat.py documentation

c6db09b

orbeckst approved these changes Jun 18, 2020

View reviewed changes

daveminh and others added 2 commits July 1, 2020 04:58

Update CHANGELOG

5153359

Merge branch 'develop' into This-is-lame

55a4b97

orbeckst self-assigned this Jul 1, 2020

orbeckst requested changes Jul 1, 2020

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

package/CHANGELOG Outdated Show resolved Hide resolved

daveminh and others added 2 commits July 1, 2020 06:57

Update package/CHANGELOG

bf26ad0

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Update CHANGELOG

7431ea2

orbeckst approved these changes Jul 1, 2020

View reviewed changes

orbeckst merged commit 8314f7d into MDAnalysis:develop Jul 1, 2020

daveminh deleted the This-is-lame branch July 1, 2020 23:22

IAlibay mentioned this pull request Jul 4, 2020

extra default.profraw file #2823

Closed

fiona-naughton added Component-Analysis new-feature labels Sep 26, 2023

IAlibay added a commit that referenced this pull request Dec 18, 2023

PR #2668

e89d99d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bond-Angle-Torsions coordinate module #2668

Bond-Angle-Torsions coordinate module #2668

daveminh commented Apr 22, 2020 •

edited

Loading

codecov bot commented Apr 22, 2020 •

edited

Loading

richardjgowers left a comment

daveminh commented Apr 23, 2020

daveminh commented Apr 23, 2020

daveminh commented Apr 23, 2020

orbeckst left a comment

orbeckst left a comment •

edited

Loading

orbeckst left a comment

orbeckst Jun 18, 2020

orbeckst commented Jun 18, 2020

orbeckst commented Jun 18, 2020 •

edited

Loading

daveminh commented Jun 19, 2020

daveminh commented Jun 24, 2020

orbeckst commented Jun 24, 2020

orbeckst commented Jul 1, 2020

orbeckst left a comment

orbeckst commented Jul 1, 2020

Bond-Angle-Torsions coordinate module #2668

Bond-Angle-Torsions coordinate module #2668

Conversation

daveminh commented Apr 22, 2020 • edited Loading

PR Checklist

codecov bot commented Apr 22, 2020 • edited Loading

Codecov Report

richardjgowers left a comment

Choose a reason for hiding this comment

daveminh commented Apr 23, 2020

daveminh commented Apr 23, 2020

daveminh commented Apr 23, 2020

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Jun 18, 2020

Choose a reason for hiding this comment

orbeckst commented Jun 18, 2020

orbeckst commented Jun 18, 2020 • edited Loading

daveminh commented Jun 19, 2020

daveminh commented Jun 24, 2020

orbeckst commented Jun 24, 2020

orbeckst commented Jul 1, 2020

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jul 1, 2020

daveminh commented Apr 22, 2020 •

edited

Loading

codecov bot commented Apr 22, 2020 •

edited

Loading

orbeckst left a comment •

edited

Loading

orbeckst commented Jun 18, 2020 •

edited

Loading