mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

Add dipole and quadrupole caclulation in AtomGroup

Open jaclark5 opened this issue 2 years ago • 2 comments

Fixes #3841

Changes made in this Pull Request:

  • Added dipole and quadrupole moments to AtomGroup topology methods

PR Checklist

  • [X] Tests?
  • [X] Docs?
  • [X] CHANGELOG updated?
  • [X] Issue raised/referenced?

jaclark5 avatar Sep 21 '22 13:09 jaclark5

Codecov Report

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

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%) :arrow_up:

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

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Sep 21 '22 14:09 codecov[bot]

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)

jaclark5 avatar Sep 22 '22 16:09 jaclark5

Sorry, I broke the docs somewhere...

orbeckst avatar Nov 29 '22 19:11 orbeckst

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.

jaclark5 avatar Dec 13 '22 16:12 jaclark5

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.

hmacdope avatar Dec 19 '22 05:12 hmacdope

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

hmacdope avatar Dec 20 '22 03:12 hmacdope

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

Ha looks like numpy 1.24 just got released and we had just been ignoring a deprecation this entire time (I'm surprised we didn't pick this up on the cron nightly builds)...

IAlibay avatar Dec 20 '22 06:12 IAlibay

These errors don't appear locally for me.

jaclark5 avatar Dec 20 '22 12:12 jaclark5

@hmacdope is the CI behaving again? Can we get this one merged?

orbeckst avatar Jan 06 '23 22:01 orbeckst

I restarted the CI.

orbeckst avatar Jan 06 '23 22:01 orbeckst

/azp rerun

orbeckst avatar Jan 06 '23 22:01 orbeckst

Command 'rerun' is not supported by Azure Pipelines.

Supported commands

  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

azure-pipelines[bot] avatar Jan 06 '23 22:01 azure-pipelines[bot]

... not sure how to kick Azure pipelines.

orbeckst avatar Jan 06 '23 22:01 orbeckst

/azp run

orbeckst avatar Jan 06 '23 22:01 orbeckst

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jan 06 '23 22:01 azure-pipelines[bot]

I think this needs to be merged with upstream/develop to get the changes in #3976 in.

hmacdope avatar Jan 07 '23 10:01 hmacdope

If someone else knows how to do that they can but otherwise I'll let @jaclark5 do it. I'll double check CI and approve when done.

hmacdope avatar Jan 07 '23 10:01 hmacdope

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

IAlibay avatar Jan 19 '23 20:01 IAlibay

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?

jaclark5 avatar Jan 20 '23 14:01 jaclark5

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 avatar Jan 20 '23 15:01 IAlibay

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.

IAlibay avatar Jan 20 '23 15:01 IAlibay