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

Add unwrap to radius_of_gyration #2299

Closed
wants to merge 3 commits into from
Closed

Add unwrap to radius_of_gyration #2299

wants to merge 3 commits into from

Conversation

NinadBhat
Copy link
Contributor

Fixes #1760

Changes made in this Pull Request:

  • Adds unwrap keyword to radius of gyration

PR Checklist

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

@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #2299 into develop will increase coverage by 1.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2299      +/-   ##
===========================================
+ Coverage     89.7%   90.74%   +1.04%     
===========================================
  Files          173       16     -157     
  Lines        21499     1989   -19510     
  Branches      2801        0    -2801     
===========================================
- Hits         19286     1805   -17481     
+ Misses        1616      184    -1432     
+ Partials       597        0     -597
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/GRO.py
package/MDAnalysis/coordinates/base.py
package/MDAnalysis/analysis/helanal.py
package/MDAnalysis/topology/MinimalParser.py
package/MDAnalysis/visualization/__init__.py
package/MDAnalysis/core/qcprot.py
...e/MDAnalysis/analysis/encore/clustering/cluster.py
package/MDAnalysis/coordinates/DCD.py
package/MDAnalysis/coordinates/GSD.py
package/MDAnalysis/coordinates/memory.py
... and 146 more

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 b1c68e4...b16acc4. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #2299      +/-   ##
==========================================
+ Coverage     89.7%   89.7%   +<.01%     
==========================================
  Files          173     173              
  Lines        21499   21506       +7     
  Branches      2801    2803       +2     
==========================================
+ Hits         19286   19293       +7     
  Misses        1616    1616              
  Partials       597     597
Impacted Files Coverage Δ
package/MDAnalysis/core/topologyattrs.py 97.82% <100%> (+0.02%) ⬆️

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 b1c68e4...53de252. Read the comment docs.

@NinadBhat
Copy link
Contributor Author

@richardjgowers this is ready to be merged.

@NinadBhat NinadBhat mentioned this pull request Jul 12, 2019
4 tasks
@NinadBhat
Copy link
Contributor Author

NinadBhat commented Jul 16, 2019

@richardjgowers @orbeckst @jbarnoud @micaela-matta This needs to be reviewed.


com = atomgroup.center_of_mass(pbc=pbc, unwrap=unwrap, compound=compound)
if compound is not 'group':
com = (com * group.masses[:, None]).sum(axis=0) / group.masses.sum()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've added a compound parameter and not written tests for it, so this line never gets tested. What is this line doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it gets tested here

If the compound is 'residue', atomgroup.center_of_mass will return COM of each residue. This line takes the mean of each COM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardjgowers is this sufficient or should I add some more tests?

@NinadBhat
Copy link
Contributor Author

I rebased this branch with develop and now the pull request is showing changes from all other commits. I have created #2305 in case I cant resolve this.

@NinadBhat NinadBhat mentioned this pull request Aug 5, 2019
4 tasks
@IAlibay IAlibay added the close? Evaluate if issue/PR is stale and can be closed. label May 18, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
close? Evaluate if issue/PR is stale and can be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radius of Gyration needs wrap option
3 participants