LinearDensity now working with residues/segments #3572

BFedder · 2022-03-24T14:14:32Z

LinearDensity did not work with grouping='residues' and grouping='segments'.
This is now fixed, and more tests have been added.

Changes made:
MDAnalysis/analysis/lineardensity.py:

in _prepare(self):
- try/except statement replaced by if statements checking the grouping
  specified for LinearDensity.
- masses and charges are obtained in a way appropriate for each case.
in _single_frame(self):
- 'positions' variable assignment changed from elem.centroid() to
  elem.atoms.centroid() so it works for residues/segments
in _conclude(self):
- Floating point arithmetic issues could lead to negative radicands. A check
  was implemented to set numbers which are close to zero to precisely zero
  before the square root (for standard deviation) is calculated.

MDAnalysisTests/analysis/test_lineardensity.py:

renamed previous test function and added expected masses and charges
added test functions for the other groupings with their respective expected
masses and charges

Regarding my suggested change to _conclude(self):
I stuck with binsize=5 for the new tests for the other groupings for internal
consistency in the tests. With this bin size, the positions of the centres of
geometry are invariant, yielding standard deviations equal to zero. Floating point
imprecision made it possible for negative values on the order of e-13 to e-14 to
appear and cause a warning from np.sqrt() though, and that is why I added the
extra step of calculating the radicand and editing it where appropriate beforehand.
Other solutions are to just ignore this and accept nan values in the result arrays,
or change the bin size for the tests. I was hesitant to do the later to stay consistent
with the established test, but I'm happy to change this of course if that's the consensus.

PR Checklist

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

LinearDensity did not work with grouping='residues' and grouping='segments'. This is now fixed, and more tests have been added. Changes made: MDAnalysis/analysis/lineardensity.py: - in _prepare(self): - try/except statement replaced by if statements checking the grouping specified for LinearDensity. - masses and charges are obtained in a way appropriate for each case. - in _single_frame(self): - 'positions' variable assignment changed from elem.centroid() to elem.atoms.centroid() so it works for residues/segments - in _conclude(self): - Floating point arithmetic issues could lead to negative radicands. A check was implemented to set numbers which are close to zero to precisely zero before the square root (for standard deviation) is calculated. MDAnalysisTests/analysis/test_lineardensity.py: - renamed previous test function and added expected masses and charges - added test functions for the other groupings with their respective expected masses and charges

pep8speaks · 2022-03-24T14:14:36Z

Hello @BFedder! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/analysis/lineardensity.py:

Line 84:80: E501 line too long (86 > 79 characters)
Line 85:80: E501 line too long (86 > 79 characters)
Line 87:1: W293 blank line contains whitespace

In the file testsuite/MDAnalysisTests/analysis/test_lineardensity.py:

Line 79:80: E501 line too long (104 > 79 characters)

Comment last updated at 2022-04-08 21:38:30 UTC

codecov · 2022-03-24T14:35:57Z

Codecov Report

Merging #3572 (565bd4d) into develop (e940bd1) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 565bd4d differs from pull request most recent head 0352033. Consider uploading reports for the commit 0352033 to get more accurate results

@@             Coverage Diff             @@
##           develop    #3572      +/-   ##
===========================================
+ Coverage    94.17%   94.22%   +0.05%     
===========================================
  Files          190      190              
  Lines        24686    24729      +43     
  Branches      3330     3320      -10     
===========================================
+ Hits         23247    23301      +54     
+ Misses        1391     1380      -11     
  Partials        48       48

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/lineardensity.py	`87.65% <100.00%> (+3.23%)`	⬆️
package/MDAnalysis/topology/MOL2Parser.py	`98.90% <0.00%> (-0.23%)`	⬇️
package/MDAnalysis/coordinates/MOL2.py	`97.63% <0.00%> (ø)`
MDAnalysisTests/coordinates/base.py	`95.35% <0.00%> (+0.25%)`	⬆️
MDAnalysisTests/auxiliary/base.py	`91.31% <0.00%> (+0.56%)`	⬆️
package/MDAnalysis/due.py	`76.66% <0.00%> (+29.99%)`	⬆️

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 993770f...0352033. Read the comment docs.

IAlibay

Couple of quick comments on the first half of the code (didn't get pass _conclude for now)

package/MDAnalysis/analysis/lineardensity.py

BFedder · 2022-03-24T15:43:55Z

Hi, and thanks for the feedback!
First off, I'll be better about checking PEP8 compliance before opening a PR next time... Sorry!

I could change the code you mention above to the following to have if/elif/else, raise an exception in the case of unknown groupings, and use the same logic for residues, segments and fragments?

# Get masses and charges for the selection
if self.grouping == "atoms":
    self.masses = self._ags[0].masses
    self.charges = self._ags[0].charges

elif self.grouping in ["residues", "segments", "fragments"]:
    self.masses = np.array([elem.atoms.total_mass() for elem in group])
    self.charges = np.array(
        [elem.atoms.total_charge() for elem in group])

else:
    raise RuntimeError(
        f"{self.grouping} is not a valid value for grouping.")

IAlibay · 2022-03-24T15:51:08Z

First off, I'll be better about checking PEP8 compliance before opening a PR next time... Sorry!

Flake8 can be really useful here, especially if combined with something like VS code. That being said, even of the coredevs are terrible at keeping up with pep8 🙄

I could change the code you mention above to the following to have if/elif/else, raise an exception in the case of unknown groupings, and use the same logic for residues, segments and fragments?

Sure, at a quick glance that seems reasonable, although you might want to use an AttributeError instead of a RuntimeError here.

IAlibay · 2022-03-24T15:51:50Z

I'm going to cycle the PR (opening and closing it), I'm not sure why gh actions is being silly today...

IAlibay · 2022-03-24T15:53:14Z

well at least the webhooks got triggered, that should be enough for now until actions comes back (another day another gh service issue... https://www.githubstatus.com/)

Changed the if/if/if for reading masses and charges to if/elif/else. Will now raise an AttributeError if invalid grouping is chosen. 'residues', 'segments' and 'fragments' now use the same logic (elem.atoms.total_mass()).

Exchanged assert_almost_equal with assert_allclose

package/MDAnalysis/analysis/lineardensity.py

Updated check of radicands in _conclude(self).

PicoCentauri

Good work to fix this!

I have some code simplifications and one physical question/addition. For me the center of geometry does not make sense to use as a reference point. It has no physical meaning.

package/MDAnalysis/analysis/lineardensity.py

testsuite/MDAnalysisTests/analysis/test_lineardensity.py

package/MDAnalysis/analysis/lineardensity.py

* Changed test of radicand to only convert negative numbers to 0 * Now uses CoM rather than CoG for density calculations * Changed Class docstring to reflect change to CoM * Now uses parameter for compound instead of list comprehension when calculating masses, charges, and CoM * refactored test_lineardensity.py, make use of pytest functionality better * Added test for case of invalid grouping selection

PicoCentauri

Very well. LGTM

Added second example to class docstring

orbeckst · 2022-04-05T18:02:09Z

@BFedder can you please resolve the conflicts?

orbeckst · 2022-04-05T18:02:27Z

@IAlibay can you please check if your comments were addressed?

IAlibay

Just two small suggested changes - I'm directly suggesting them here since they are more "my style" than anything. Feel free to say no, I'll approve after.

package/MDAnalysis/analysis/lineardensity.py

testsuite/MDAnalysisTests/analysis/test_lineardensity.py

Co-authored-by: Irfan Alibay <[email protected]>

BFedder · 2022-04-08T17:51:26Z

Thanks, I have taken the suggestions on board

IAlibay

Thanks @BFedder

@MDAnalysis/gsoc-mentors - I can't merge this as a GSoC PR due to conflicts of interest. Can someone please take over the responsibility of merging this?

orbeckst · 2022-04-08T19:48:47Z

@PicoCentauri can you please have a final look and then squash-merge? Thanks.

PicoCentauri · 2022-04-08T21:44:35Z

Thanks @BFedder for your fix!

github-actions bot added the Component-Analysis label Mar 24, 2022

IAlibay requested changes Mar 24, 2022

View reviewed changes

IAlibay closed this Mar 24, 2022

IAlibay reopened this Mar 24, 2022

changes requested by IAlibay

d96245e

Changed the if/if/if for reading masses and charges to if/elif/else. Will now raise an AttributeError if invalid grouping is chosen. 'residues', 'segments' and 'fragments' now use the same logic (elem.atoms.total_mass()).

BFedder force-pushed the lindens branch from 22c4cc9 to d96245e Compare March 24, 2022 17:39

BFedder added 4 commits March 24, 2022 17:43

Update test_lineardensity.py

5476840

Update lineardensity.py

8b23b5d

Update test_lineardensity.py

ba62953

Exchanged assert_almost_equal with assert_allclose

Update CHANGELOG

5e51f5a

RMeli reviewed Mar 25, 2022

View reviewed changes

package/MDAnalysis/analysis/lineardensity.py Outdated Show resolved Hide resolved

package/MDAnalysis/analysis/lineardensity.py Outdated Show resolved Hide resolved

IAlibay and others added 2 commits March 30, 2022 13:45

Merge branch 'develop' into lindens

b642918

Update lineardensity.py

0aa743e

Updated check of radicands in _conclude(self).

orbeckst requested a review from PicoCentauri April 2, 2022 00:23

PicoCentauri requested changes Apr 2, 2022

View reviewed changes

package/MDAnalysis/analysis/lineardensity.py Outdated Show resolved Hide resolved

package/MDAnalysis/analysis/lineardensity.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/analysis/test_lineardensity.py Outdated Show resolved Hide resolved

IAlibay requested changes Apr 2, 2022

View reviewed changes

package/MDAnalysis/analysis/lineardensity.py Show resolved Hide resolved

package/MDAnalysis/analysis/lineardensity.py Outdated Show resolved Hide resolved

BFedder and others added 2 commits April 4, 2022 15:23

Update lineardensity.py

ea4e561

PicoCentauri approved these changes Apr 4, 2022

View reviewed changes

Update lineardensity.py

9d2eb68

Added second example to class docstring

BFedder mentioned this pull request Apr 5, 2022

LinearDensity should have more reliable tests #3603

Open

orbeckst assigned orbeckst and PicoCentauri Apr 5, 2022

orbeckst removed their assignment Apr 5, 2022

Merge branch 'develop' into lindens

dd6c338

IAlibay requested changes Apr 8, 2022

View reviewed changes

package/MDAnalysis/analysis/lineardensity.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/analysis/test_lineardensity.py Outdated Show resolved Hide resolved

BFedder and others added 2 commits April 8, 2022 18:49

Update package/MDAnalysis/analysis/lineardensity.py

446de45

Co-authored-by: Irfan Alibay <[email protected]>

Update testsuite/MDAnalysisTests/analysis/test_lineardensity.py

565bd4d

Co-authored-by: Irfan Alibay <[email protected]>

IAlibay approved these changes Apr 8, 2022

View reviewed changes

PicoCentauri approved these changes Apr 8, 2022

View reviewed changes

Merge branch 'develop' into lindens

0352033

PicoCentauri merged commit 7c166c0 into MDAnalysis:develop Apr 8, 2022

IAlibay mentioned this pull request Apr 9, 2022

Fix docs code block for lineardensity #3616

Merged

1 task

BFedder deleted the lindens branch April 10, 2022 09:17

IAlibay added enhancement defect labels Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LinearDensity now working with residues/segments #3572

LinearDensity now working with residues/segments #3572

BFedder commented Mar 24, 2022

pep8speaks commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 24, 2022 •

edited

Loading

IAlibay left a comment

BFedder commented Mar 24, 2022

IAlibay commented Mar 24, 2022

IAlibay commented Mar 24, 2022

IAlibay commented Mar 24, 2022

PicoCentauri left a comment

PicoCentauri left a comment

orbeckst commented Apr 5, 2022

orbeckst commented Apr 5, 2022

IAlibay left a comment

BFedder commented Apr 8, 2022

IAlibay left a comment

orbeckst commented Apr 8, 2022

PicoCentauri commented Apr 8, 2022

LinearDensity now working with residues/segments #3572

LinearDensity now working with residues/segments #3572

Conversation

BFedder commented Mar 24, 2022

PR Checklist

pep8speaks commented Mar 24, 2022 • edited Loading

Comment last updated at 2022-04-08 21:38:30 UTC

codecov bot commented Mar 24, 2022 • edited Loading

Codecov Report

IAlibay left a comment

Choose a reason for hiding this comment

BFedder commented Mar 24, 2022

IAlibay commented Mar 24, 2022

IAlibay commented Mar 24, 2022

IAlibay commented Mar 24, 2022

PicoCentauri left a comment

Choose a reason for hiding this comment

PicoCentauri left a comment

Choose a reason for hiding this comment

orbeckst commented Apr 5, 2022

orbeckst commented Apr 5, 2022

IAlibay left a comment

Choose a reason for hiding this comment

BFedder commented Apr 8, 2022

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst commented Apr 8, 2022

PicoCentauri commented Apr 8, 2022

pep8speaks commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 24, 2022 •

edited

Loading