Add pickling of Atom, Residue, Segment and groups. #3953

pgbarletta · 2022-12-08T13:51:05Z

From #3947

Changes made in this Pull Request:

Add reduce() method to both ResidueGroup and SegmentGroup
Change _unpickle() so it can create any kind of group from its indices.
Add one test for each group in the parallelism section.

PR Checklist

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

PD: sorry for the new PR. I synced up my fork from develop and that somehow closed the previous PR.

codecov · 2022-12-08T14:07:46Z

Codecov Report

Base: 93.48% // Head: 93.49% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (404cd45) compared to base (0c4f823).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3953   +/-   ##
========================================
  Coverage    93.48%   93.49%           
========================================
  Files          190      190           
  Lines        24951    24963   +12     
  Branches      3527     3527           
========================================
+ Hits         23326    23338   +12     
  Misses        1102     1102           
  Partials       523      523

Impacted Files	Coverage Δ
package/MDAnalysis/core/groups.py	`97.51% <100.00%> (+0.02%)`	⬆️

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.

package/MDAnalysis/core/groups.py

richardjgowers

can we use empty Universes?

richardjgowers · 2022-12-14T11:42:39Z

testsuite/MDAnalysisTests/core/test_segmentgroup.py

+    """Test SegmentGroup pickling support."""
+    @pytest.fixture()
+    def universe(self):
+        return mda.Universe(TPR, XTC)


minor quibble, but if you use Universe.empty() it'll make the tests a little faster. it's not a big deal, but over 1,000 tests it adds up slowly

Sorry, don't get it. Say I do:

seg_group = mda.Universe.empty(10).segments seg = pickle.loads(pickle.dumps(seg_group))

how do I compare them? And would that be a useful test?

you could check that the new segments have the same universe as the old one:

u = mda.Universe.empty() segs = u.segments new = pickle.loads(...) assert new.universe is u

I think maybe you're right, a better test would have dummy data inside the Universe...

u = mda.Universe.empty() u.add_TopologyAttr('segids', values=['A', 'B', etc] ... some pickling assert new.segids == ['A', 'B', etc]

The idea is you can create a Universe without using files, and it'll make the tests faster if we don't (and also makes the tests not reliant on TPRParser working)

thanks. I went with the 2nd choice:

def test_segmentgroup_pickle(): u = mda.Universe.empty(10) u.add_Segment(segid="X") u.add_Segment(segid="Y") u.add_Segment(segid="Z") segids = ["A", "X", "Y", "Z"] u.add_TopologyAttr("segids", values=["A", "X", "Y", "Z"]) seg_group = mda.SegmentGroup((1, 3), u) seg = pickle.loads(pickle.dumps(seg_group)) assert_equal(seg.universe.segments.segids, segids)

hmacdope

Two little things and then I think good to go.

package/MDAnalysis/core/groups.py

hmacdope · 2022-12-20T03:10:55Z

package/CHANGELOG

@@ -39,6 +39,8 @@ Fixes
  * fixes guessed_attributes and read_attributes methods of the Topology class, as 
    those methods were giving unexpected results for some topology attributes
    (e.g. bonds, angles) (PR #3779).
+  * Add pickling support for Atom, Residue, Segment, ResidueGroup 


Entries newest first and this needs to be put under 2.5.0. Merging with upstream/develop also required. :)

hmacdope

Two tiny nitpicks sorry.

package/AUTHORS

package/CHANGELOG

pgbarletta · 2022-12-23T09:29:38Z

I'll be out for a ~week. I'll rebase once the numpy deprecations issues are fixed.

hmacdope

LGTM thanks @pgbarletta

package/CHANGELOG

Add reduce() method to both ResidueGroup and SegmentGroup Change _unpickle() so it can create any kind of group from its indices. Add one test for each group in the parallelism section.

IAlibay · 2023-01-03T14:59:39Z

package/MDAnalysis/core/groups.py

@@ -119,6 +119,11 @@ def _unpickle(u, ix):
    return u.atoms[ix]


+# TODO 3.0: deprecate _unpickle in favor of _unpickle2.


Just a comment here, unless it changes user-facing behaviour there is no need to deprecate a private method like this. We can just throw it out right now and none would be the wiser.

Please raise a relevant issue regarding this though.

IAlibay

I'll approve although please do raise the issue for the follow-up 3.0 work, otherwise it'll get lost.

github-actions bot added the Component-Core label Dec 8, 2022

pgbarletta mentioned this pull request Dec 8, 2022

Add ResidueGroup and SegmentGroup pickling. #3948

Closed

4 tasks

richardjgowers reviewed Dec 14, 2022

View reviewed changes

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

richardjgowers requested changes Dec 14, 2022

View reviewed changes

richardjgowers approved these changes Dec 16, 2022

View reviewed changes

hmacdope requested changes Dec 20, 2022

View reviewed changes

hmacdope requested changes Dec 23, 2022

View reviewed changes

package/AUTHORS Outdated Show resolved Hide resolved

hmacdope reviewed Dec 23, 2022

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

github-actions bot added Component-Analysis Continuous Integration and removed Component-Analysis Continuous Integration labels Jan 2, 2023

pgbarletta requested a review from hmacdope January 2, 2023 11:32

hmacdope approved these changes Jan 2, 2023

View reviewed changes

IAlibay requested changes Jan 2, 2023

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

Add pickling of Atom, Residue, Segment and groups.

404cd45

Add reduce() method to both ResidueGroup and SegmentGroup Change _unpickle() so it can create any kind of group from its indices. Add one test for each group in the parallelism section.

pgbarletta requested a review from IAlibay January 3, 2023 08:29

IAlibay reviewed Jan 3, 2023

View reviewed changes

IAlibay approved these changes Jan 3, 2023

View reviewed changes

hmacdope merged commit d67b4f1 into MDAnalysis:develop Jan 3, 2023

orbeckst mentioned this pull request Feb 9, 2023

Add support for pickling of ResidueGroup and SegmentGroup #3947

Closed

IAlibay added the enhancement label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pickling of Atom, Residue, Segment and groups. #3953

Add pickling of Atom, Residue, Segment and groups. #3953

pgbarletta commented Dec 8, 2022 •

edited by hmacdope

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading

richardjgowers left a comment

richardjgowers Dec 14, 2022

pgbarletta Dec 14, 2022

richardjgowers Dec 15, 2022

pgbarletta Dec 15, 2022

hmacdope left a comment

hmacdope Dec 20, 2022

hmacdope left a comment

pgbarletta commented Dec 23, 2022

hmacdope left a comment

IAlibay Jan 3, 2023

pgbarletta Jan 3, 2023

IAlibay left a comment

		@@ -119,6 +119,11 @@ def _unpickle(u, ix):
		return u.atoms[ix]


		# TODO 3.0: deprecate _unpickle in favor of _unpickle2.

Add pickling of Atom, Residue, Segment and groups. #3953

Add pickling of Atom, Residue, Segment and groups. #3953

Conversation

pgbarletta commented Dec 8, 2022 • edited by hmacdope Loading

PR Checklist

codecov bot commented Dec 8, 2022 • edited Loading

Codecov Report

richardjgowers left a comment

Choose a reason for hiding this comment

richardjgowers Dec 14, 2022

Choose a reason for hiding this comment

pgbarletta Dec 14, 2022

Choose a reason for hiding this comment

richardjgowers Dec 15, 2022

Choose a reason for hiding this comment

pgbarletta Dec 15, 2022

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope Dec 20, 2022

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

pgbarletta commented Dec 23, 2022

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay Jan 3, 2023

Choose a reason for hiding this comment

pgbarletta Jan 3, 2023

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

pgbarletta commented Dec 8, 2022 •

edited by hmacdope

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading