mdanalysis
mdanalysis copied to clipboard
Add dipole and quadrupole caclulation in AtomGroup
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?
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.
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)
Sorry, I broke the docs somewhere...
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.
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.
Actually I'm going to block here on CI issues, @jaclark5 can you reproduce these locally?
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)...
These errors don't appear locally for me.
@hmacdope is the CI behaving again? Can we get this one merged?
I restarted the CI.
/azp rerun
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.
... not sure how to kick Azure pipelines.
/azp run
Azure Pipelines successfully started running 1 pipeline(s).
I think this needs to be merged with upstream/develop
to get the changes in #3976 in.
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.
Went ahead and merged - should be good to merge once CI is green
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?
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).
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.