Add dipole and quadrupole caclulation in AtomGroup #3842

jaclark5 · 2022-09-21T13:57:36Z

Fixes #3841

Changes made in this Pull Request:

Added dipole and quadrupole moments to AtomGroup topology methods

PR Checklist

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

codecov · 2022-09-21T14:17:47Z

Codecov Report

Base: 93.50% // Head: 93.52% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (f3ee76f) compared to base (363037a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3842      +/-   ##
===========================================
+ Coverage    93.50%   93.52%   +0.02%     
===========================================
  Files          190      190              
  Lines        24943    25028      +85     
  Branches      3523     3542      +19     
===========================================
+ Hits         23322    23407      +85     
  Misses        1100     1100              
  Partials       521      521

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

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

hmacdope

Hi @jaclark5, another awesome contribution that is coming along super nicely. I think this would be a well used and useful addition. 🥇

This is mostly a review for style rather than scientific correctness, I will try and circle back for a more detailed look ASAP.

One thing that would be great is if you could make some test cases that are easy to reason about, eg dipole/quadruplepole moments that should be zero by virtue of equally separated charges, or moments that should all be a certain value etc etc.

package/MDAnalysis/core/topologyattrs.py

package/doc/sphinx/source/documentation_pages/references.rst

testsuite/MDAnalysisTests/core/test_accumulate.py

orbeckst

Thanks for the contribution, this looks quite useful. I'd advocate for extending it to also return dipolevector and quadrupoletensors instead of just scalar moments. Otherwise see various comments on notation. Sorry, did not have time to look at code in depth.

package/MDAnalysis/core/topologyattrs.py

jaclark5 · 2022-09-22T16:55:53Z

Thanks, I do think returning the quadrupole moment is something that must happen. Most people calculate the quadrupole moment with just the diagonal elements and it's only Gray and Gubbins that specify this method of calculating it will all information in the tensor. Additionally, the quadrupole moment can not be easily adjusted for it's origin and is very origin dependent. All these factors are a lot to handle for a user. I'm willing to add two additional dipole_vector, and quadrupole_tensor though (and then change the definition of dipole_moment)

hmacdope

Looking great, see my comments. :)

hmacdope · 2022-11-20T04:19:35Z

maintainer/conda/environment.yml

@@ -27,6 +27,7 @@ dependencies:
  - tqdm>=4.43.0
  - sphinxcontrib-bibtex
  - sphinx_rtd_theme
+  - pyyaml>=3.02


@jaclark5 what is this addition for exactly? I might have missed it.

The new citation method won't compile without this constraint.

package/CHANGELOG

package/MDAnalysis/core/topologyattrs.py

hmacdope · 2022-11-20T04:27:25Z

package/requirements.txt

@@ -15,6 +15,7 @@ numpy>=1.20.0
 packaging
 parmed
 pytest
+pyyaml>=3.02


Why the pyyaml addition?

I added this constraint to all files that check for the sphinx installation since this constraint allows the citations to be compiled.

Im surprised the docs arn't working normally...

Perhaps fix the CHANGELOG conflicts and re-trigger CI and I'll have a look.

So to be clear, you want me to remove this change and then run the CI so you can see the error? I'm 90% sure that the failure in #3905 is this same failure, so I could also add this change to that PR to show that it's resolved.

@hmacdope I tried to look through my browsing history to find the site where I found the answer, but I didn't find it. I thiiiink I took a shot in the dark based on this statement (https://avocado-framework.readthedocs.io/en/latest/releases/44_0.html?highlight=pyyaml#documentation) and it worked.

Here is the link to the check-logs for this PR where you see the same errors that are now presenting in #3905, illustrating that the issue is consistent between the two.https://github.com/MDAnalysis/mdanalysis/actions/runs/3159210367/jobs/5142154816

Also I did give a heads up about this to the citation work that is under development in Oct as seen here:
MDAnalysis/UserGuide#212

Sorry I'm coming here a bit blind to this whole issue, but why requirements.txt? Is this for an issue with running things locally?

Re: CI, as far as I'm aware either uses optional deps defined in the github action or the environment.yaml file under maintainer.

It was for an issue with compiling the docs with CI, and I can't remember if I had the same issue locally. Although it appears that this is not longer an issue since the other PR with the same issue compiled the docs in CI (#3905). There must have been a change somewhere within the past week that resolved it in a different way.

I'll remove the commits that added these dependencies.

Ok in theory I should be able to remove the two commits involved in this thread (first to add the dependencies and then to remove them). However, I keep making a mess, could I get some advice?

git rebase -i HEAD~30, this shows a list of all previous commits from both me and the develop branch.

At the commits of interest, I change "pick" to "drop" and then save and quit in vi

A conflict in the CHANGELOG appears (like always) and asks me to change it. If I do, git adds the change to someone else's commit, so instead I type git rebase --skip a million times and the conflict is eventually resolved down the line.

I can then force push these changes to GitHub, however, all commits from the development branch now show up on this PR with the label "@soandso authored, @jaclark5 committed". I don't like this so then I reflog locally to undo everything and force push that.

Is there something I should be doing differently?

We usually squash merge PRs, so history doesn't usually end up being an issue, would this fix things here?

Hey, if you mean that squashing the commits will cancel out the two that I want to remove, then yes :). Ok I think this is good to go then if I've addressed all of @hmacdope's concerns (I think I have).

testsuite/MDAnalysisTests/core/test_accumulate.py

orbeckst

@jaclark5 I have a bunch of small nit-picks on the formatting and style.

PEP8 — please leave space after comma, no space after assignment in argument list
when boldfacing variable names as vectors, please do not boldface subscripts
please be clear about semantics of equations and just say in words what operations mean because not everyone is using the same symbols for operations

I really like the PR, I just don't want to have to come back to it later and fix up little things. Thank you!

package/MDAnalysis/core/topologyattrs.py

orbeckst

Thank you for addressing my nit-picks. I added a bunch of final text fixes and I'll commit them myself in a moment and when all passes and I didn't break the docs then I'll approve.

package/MDAnalysis/core/topologyattrs.py

orbeckst · 2022-11-29T19:59:09Z

Sorry, I broke the docs somewhere...

hmacdope

Looks super good just one or two nitpicks. One last annoying request, could we have a toy test with 0 dipole and quadrupole by virtue of symmetry? Would be easy to reason about.

package/CHANGELOG

package/MDAnalysis/core/topologyattrs.py

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

Hey I'll get to these @hmacdope, but I did add a case with exactly zero dipole and quadrupole, namely methane in the tests. Does that not satisfy your intention? Otherwise, I'm going to need a better idea of what you expect.

hmacdope · 2022-12-19T05:20:21Z

Hey I'll get to these @hmacdope, but I did add a case with exactly zero dipole and quadrupole, namely methane in the tests. Does that not satisfy your intention? Otherwise, I'm going to need a better idea of what you expect.

No spot on. All good from my end! Just update for new CHANGELOG. I'll approve ~~now~~ when changelog is fixed, and merge in a few days if no other comments from @MDAnalysis/coredevs.

package/MDAnalysis/core/topologyattrs.py

hmacdope

I'm going to approve here as my comments have been addressed. See @IAlibay's outstanding review.

hmacdope · 2022-12-20T03:04:39Z

Actually I'm going to block here on CI issues, @jaclark5 can you reproduce these locally?

…mmetry

hmacdope

LGTM! Thanks for the great contribution.

testsuite/MDAnalysisTests/core/test_accumulate.py

IAlibay · 2023-01-19T20:37:59Z

Went ahead and merged - should be good to merge once CI is green

jaclark5 · 2023-01-20T14:25:29Z

The error is: testsuite/MDAnalysisTests/datafiles.py:288:89: E501 line too long (89 > 88 characters) [flake8]

This line isn't part of this contribution, but should I fix it anyway?

IAlibay · 2023-01-20T15:35:23Z

The error is: testsuite/MDAnalysisTests/datafiles.py:288:89: E501 line too long (89 > 88 characters) [flake8]

This line isn't part of this contribution, but should I fix it anyway?

Nope, you can ignore that, datafiles is one of those we don't care about re: PEP8. Darken doesn't know not to look at certain files (I think we can set up an file with some excludes, we probably should do that eventually).

IAlibay · 2023-01-20T15:36:04Z

I'm just going to cycle CI again since we just merged a bunch of stuff, if that returns green I think it's good to go.

github-actions bot added the Component-Core label Sep 21, 2022

jaclark5 force-pushed the multipole branch from 60447ed to f96a736 Compare September 21, 2022 14:00

hmacdope requested changes Sep 22, 2022

View reviewed changes

orbeckst reviewed Sep 22, 2022

View reviewed changes

github-actions bot added the Continuous Integration label Oct 11, 2022

jaclark5 force-pushed the multipole branch from 6414a93 to 895dba6 Compare October 11, 2022 22:30

jaclark5 marked this pull request as ready for review October 21, 2022 14:43

jaclark5 requested a review from hmacdope October 21, 2022 14:44

hmacdope requested changes Nov 20, 2022

View reviewed changes

jaclark5 mentioned this pull request Nov 29, 2022

gyration_moments method / allow shape parameters to yield per residue quantities #3905

Merged

4 tasks

orbeckst requested changes Nov 29, 2022

View reviewed changes

orbeckst assigned hmacdope Nov 29, 2022

orbeckst approved these changes Nov 29, 2022

View reviewed changes

hmacdope mentioned this pull request Dec 2, 2022

[DOC]: Updated references to BibTex format #3775

Open

2 tasks

github-actions bot removed the Continuous Integration label Dec 7, 2022

jaclark5 force-pushed the multipole branch from 26beb62 to d6ec345 Compare December 7, 2022 20:36

github-actions bot added the Continuous Integration label Dec 7, 2022

jaclark5 force-pushed the multipole branch from d6ec345 to 9218e16 Compare December 7, 2022 20:38

github-actions bot removed the Continuous Integration label Dec 7, 2022

hmacdope requested changes Dec 13, 2022

View reviewed changes

hmacdope self-requested a review December 19, 2022 05:20

IAlibay requested changes Dec 20, 2022

View reviewed changes

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

hmacdope approved these changes Dec 20, 2022

View reviewed changes

jaclark5 added 19 commits January 8, 2023 16:30

Update multipole tests

f00bbc2

Added multipole test data files, revised quadrupole matrix operation

f4c2c87

Update Multipole moment doc strings

dcf0bde

Added use of trace in quadrupole_tensor

4f80d04

Update citation

11a96cf

Added pyyaml dependency for docs

41c7052

Bug fix center_of_charge

8a04753

Update multipole tests for full coverage

5b6621b

Integrate reviewer comments

87855c1

Update docs

239bc30

Addressed reviewer comments

e006aa4

Update equations for dipole_vector

c7f20dc

change methane in multipole tests to have coordinates with perfect sy…

d8b711d

…mmetry

Updated documentation to multipole

7906657

Updated quadrupole tensor doc string and pep8 adherence

213867b

Remove pyyaml dependency

c0e97ca

Updates to reviewer comments

50da80f

Removed irrelevant pep8 corrections

658389a

mend

523518b

jaclark5 force-pushed the multipole branch from 52b92cc to 523518b Compare January 8, 2023 21:34

jaclark5 requested a review from hmacdope January 17, 2023 13:44

hmacdope approved these changes Jan 19, 2023

View reviewed changes

IAlibay reviewed Jan 19, 2023

View reviewed changes

testsuite/MDAnalysisTests/core/test_accumulate.py Outdated Show resolved Hide resolved

Update testsuite/MDAnalysisTests/core/test_accumulate.py

f3ee76f

hmacdope merged commit d73523f into MDAnalysis:develop Jan 24, 2023

IAlibay added the enhancement label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dipole and quadrupole caclulation in AtomGroup #3842

Add dipole and quadrupole caclulation in AtomGroup #3842

jaclark5 commented Sep 21, 2022

codecov bot commented Sep 21, 2022 •

edited

hmacdope left a comment

orbeckst left a comment

jaclark5 commented Sep 22, 2022

hmacdope left a comment

hmacdope Nov 20, 2022

jaclark5 Nov 22, 2022

hmacdope Nov 20, 2022

jaclark5 Nov 22, 2022

hmacdope Nov 27, 2022

jaclark5 Nov 29, 2022

jaclark5 Dec 7, 2022 •

edited

IAlibay Dec 7, 2022

jaclark5 Dec 7, 2022 •

edited

jaclark5 Dec 7, 2022

IAlibay Dec 7, 2022

jaclark5 Dec 8, 2022

orbeckst left a comment

orbeckst left a comment

orbeckst commented Nov 29, 2022

hmacdope left a comment

jaclark5 commented Dec 13, 2022

hmacdope commented Dec 19, 2022 •

edited

hmacdope left a comment

hmacdope commented Dec 20, 2022

hmacdope left a comment

IAlibay commented Jan 19, 2023

jaclark5 commented Jan 20, 2023

IAlibay commented Jan 20, 2023

IAlibay commented Jan 20, 2023

Add dipole and quadrupole caclulation in AtomGroup #3842

Add dipole and quadrupole caclulation in AtomGroup #3842

Conversation

jaclark5 commented Sep 21, 2022

PR Checklist

codecov bot commented Sep 21, 2022 • edited

Codecov Report

hmacdope left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

jaclark5 commented Sep 22, 2022

hmacdope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaclark5 Dec 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaclark5 Dec 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Nov 29, 2022

hmacdope left a comment

Choose a reason for hiding this comment

jaclark5 commented Dec 13, 2022

hmacdope commented Dec 19, 2022 • edited

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope commented Dec 20, 2022

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay commented Jan 19, 2023

jaclark5 commented Jan 20, 2023

IAlibay commented Jan 20, 2023

IAlibay commented Jan 20, 2023

codecov bot commented Sep 21, 2022 •

edited

jaclark5 Dec 7, 2022 •

edited

jaclark5 Dec 7, 2022 •

edited

hmacdope commented Dec 19, 2022 •

edited