gyration_moments method / allow shape parameters to yield per residue quantities by jaclark5 · Pull Request #3905 · MDAnalysis/mdanalysis

jaclark5 · 2022-11-04T21:25:27Z

… per residue quantities

Fixes #3002
Fixes #3904

Changes made in this Pull Request:

Added AtomGroup Method to provide group and per residue moments of gyration.

PR Checklist

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

codecov · 2022-11-04T21:42:58Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ec2f2c0) 93.56% compared to head (6782962) 93.56%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3905   +/-   ##
========================================
  Coverage    93.56%   93.56%           
========================================
  Files          191      191           
  Lines        25065    25075   +10     
  Branches      4042     4049    +7     
========================================
+ Hits         23452    23462   +10     
  Misses        1093     1093           
  Partials       520      520

Impacted Files	Coverage Δ
package/MDAnalysis/core/topologyattrs.py	`96.12% <100.00%> (+0.03%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

hmacdope

Few comments and queries. :)

package/MDAnalysis/core/topologyattrs.py

testsuite/MDAnalysisTests/core/test_atomgroup.py

hmacdope · 2022-11-27T01:08:01Z

@jaclark5 would you be able to fix conflicts / trigger CI and we can go from there? Might also be good to have another pair of eyes @MDAnalysis/coredevs?

jaclark5 · 2022-11-29T02:57:56Z

@hmacdope These errors occur from the new Bibtex citation style. This would be resolved with the following changes:

maintainer/conda/environment.yml:  - pyyaml>=3.02
package/requirements.txt:pyyaml>=3.02

Recall that these changes were added so that the tests would pass in my other PR #3842. So as soon as those are merged, I'll update this PR with the develop branch ;)

jaclark5 · 2022-12-07T16:19:27Z

@hmacdope I'm now getting an error in the tests from MDAnalysis/coordinate/XTC.py

IAlibay · 2022-12-07T16:23:12Z

I'm now getting an error in the tests from MDAnalysis/coordinate/XTC.py

@jaclark5 are you getting this locally? If you so you might need to pip install MDAnalysis again.

jaclark5 · 2022-12-07T16:26:43Z

I am getting it locally. Good point, thanks.

Edit: That worked

orbeckst · 2023-01-06T22:39:30Z

/azp run

azure-pipelines · 2023-01-06T22:39:39Z

Azure Pipelines successfully started running 1 pipeline(s).

hmacdope

Looking good just a few little things :)

package/MDAnalysis/core/topologyattrs.py

hmacdope

LGTM @jaclark5, thanks for another great contribution. @MDAnalysis/coredevs could I get a second review here?

… per residue quantities

hmacdope · 2023-02-16T17:17:41Z

@MDAnalysis/coredevs still looking for a second review here. :)

hmacdope · 2023-02-26T08:33:10Z

@MDAnalysis/coredevs still looking for a second review here. :)

Bump on this @MDAnalysis/coredevs?

orbeckst

Thanks for the fix and code consolidation. I had a quick look through and mainly noticed doc things.

Did we have tests for the case where the methods actually worked as expected and do these tests still produce the same values?

package/MDAnalysis/core/topologyattrs.py

orbeckst · 2023-03-02T01:26:34Z

package/MDAnalysis/core/topologyattrs.py

           Renamed `pbc` kwarg to `wrap`. `pbc` is still accepted but
           is deprecated and will be removed in version 3.0.
+        .. versionchanged:: 2.5.0
+           Added use of gyration_moments for per residue quantities


Is this important for the user to know how the quantities are computed? I'd assume the more important message is what you have in the CHANGELOG: since 2.5.0 you can calculate asphericity for any compound.

That's what I was going for with "for per residue quantities" but hopeful the change is more clear.

package/MDAnalysis/core/topologyattrs.py

jaclark5 · 2023-03-02T19:59:50Z

I kept the same testing structure that existed, some results stayed the same and others changed. For a calculation with the entire atom group, the result stayed the same (line 1098, 1120 in the test file.). In the tests where other compound options were used, the result changed as there is more than one value each reflecting a residue, while before each instance had the same value as when there was no compound specified.

orbeckst · 2023-03-15T17:32:57Z

sorry, I currently only have very limited time and won't be able to review soon

orbeckst · 2023-03-15T17:33:47Z

actually... that should be quick

hmacdope · 2023-03-15T22:21:16Z

Brilliant! two reviews. I will merge now.

github-actions bot added the Component-Core label Nov 4, 2022

hmacdope requested changes Nov 20, 2022

View reviewed changes

hmacdope mentioned this pull request Nov 21, 2022

Add "unwrap" keyword to shape_parameter #2292

Closed

hmacdope linked an issue Nov 21, 2022 that may be closed by this pull request

Add "unwrap" keyword to shape_parameter #2292

Closed

jaclark5 mentioned this pull request Nov 29, 2022

Add dipole and quadrupole caclulation in AtomGroup #3842

Merged

4 tasks

hmacdope requested changes Jan 26, 2023

View reviewed changes

package/MDAnalysis/core/topologyattrs.py Outdated Show resolved Hide resolved

package/MDAnalysis/core/topologyattrs.py Outdated Show resolved Hide resolved

package/MDAnalysis/core/topologyattrs.py Outdated Show resolved Hide resolved

jaclark5 requested a review from hmacdope February 7, 2023 14:43

hmacdope approved these changes Feb 7, 2023

View reviewed changes

jaclark5 changed the title ~~gyration_moments method / allow shape parameters to yield per residue quantities~~ gyration_moments method / allow shape parameters to yield per residue quantities 1 Feb 9, 2023

jaclark5 changed the title ~~gyration_moments method / allow shape parameters to yield per residue quantities 1~~ gyration_moments method / allow shape parameters to yield per residue quantities Feb 9, 2023

jaclark5 added 5 commits February 9, 2023 11:57

Added gyration_moments method which allows shapre parameters to yield…

40662e2

… per residue quantities

Update docs for gyration tensor

ef1ce0b

Bugfix in topologyattr

d764e4d

Added test for codecov

76640c3

Update added/changed version

3899bdd

jaclark5 force-pushed the gyration_eig branch from 8a46df5 to 3899bdd Compare February 9, 2023 16:58

jaclark5 and others added 2 commits February 9, 2023 12:15

Update Changelog

a6c9986

Merge branch 'develop' into gyration_eig

273793e

jaclark5 added 2 commits February 26, 2023 11:02

Merge branch 'develop' into gyration_eig

60a3798

Merge branch 'develop' into gyration_eig

9359584

orbeckst requested changes Mar 2, 2023

View reviewed changes

Response to review

b3cce4d

jaclark5 and others added 3 commits March 2, 2023 15:00

Merge branch 'develop' into gyration_eig

acd8ecf

Bug fix

13e1428

Merge branch 'develop' into gyration_eig

bd5fab0

jaclark5 requested a review from orbeckst March 15, 2023 13:08

Merge branch 'develop' into gyration_eig

6782962

orbeckst approved these changes Mar 15, 2023

View reviewed changes

hmacdope merged commit dd234c1 into MDAnalysis:develop Mar 15, 2023

jaclark5 deleted the gyration_eig branch March 17, 2023 14:48

IAlibay added the enhancement label Sep 25, 2023

Conversation

jaclark5 commented Nov 4, 2022

PR Checklist

Uh oh!

codecov bot commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hmacdope left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmacdope commented Nov 27, 2022

Uh oh!

jaclark5 commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclark5 commented Dec 7, 2022

Uh oh!

IAlibay commented Dec 7, 2022

Uh oh!

jaclark5 commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst commented Jan 6, 2023

Uh oh!

azure-pipelines bot commented Jan 6, 2023

Uh oh!

hmacdope left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmacdope left a comment

Choose a reason for hiding this comment

Uh oh!

hmacdope commented Feb 16, 2023

Uh oh!

hmacdope commented Feb 26, 2023

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

jaclark5 Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaclark5 commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst commented Mar 15, 2023

Uh oh!

orbeckst commented Mar 15, 2023

Uh oh!

hmacdope commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 4, 2022 •

edited

Loading

jaclark5 commented Nov 29, 2022 •

edited

Loading

jaclark5 commented Dec 7, 2022 •

edited

Loading

jaclark5 Mar 2, 2023 •

edited

Loading

jaclark5 commented Mar 2, 2023 •

edited

Loading