No need to redirect to the original frame explicitly during deserialization #3722

yuxuanzhuang · 2022-06-19T18:26:16Z

Fixes #3723
Partial fix: #3721

Changes made in this Pull Request:

no redirection to the original frame (explicitly) during deserialization
TextIOPicklable serializes the raw class instance instead of class name.

PR Checklist

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

pep8speaks · 2022-06-19T18:26:19Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-04-03 14:10:03 UTC

codecov · 2022-06-19T22:23:18Z

Codecov Report

Attention: Patch coverage is 95.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.64%. Comparing base (73acc9b) to head (1a6272e).

Files	Patch %	Lines
package/MDAnalysis/lib/picklable_file_io.py	94.44%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3722      +/-   ##
===========================================
- Coverage    93.66%   93.64%   -0.02%     
===========================================
  Files          168      180      +12     
  Lines        21248    22332    +1084     
  Branches      3917     3918       +1     
===========================================
+ Hits         19902    20913    +1011     
- Misses         888      961      +73     
  Partials       458      458

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

orbeckst · 2022-06-22T17:13:51Z

Once you need someone to review, feel free to ping/request reviewers.

I'd also suggest using the "convert to draft" in GitHub instead of just adding "WIP" to the issue title as it shows more intentionally if you currently want reviews or not.

richardjgowers

This is probably an improvement, but I think it'll be a change in behaviour so we'll need to raise warnings etc.

yuxuanzhuang · 2022-06-30T12:02:11Z

This is probably an improvement, but I think it'll be a change in behaviour so we'll need to raise warnings etc.

The behaviour change is how the reading location TextIOPicklable is recovered. Before it is done by universe.trajectory[i]. Now it is directly serialized. Not sure if there's any explicitly warning that should be raised anywhere?

richardjgowers

@yuxuanzhuang sorry for leaving this for a while. So is the behaviour now that serialising/deserialising a Reader preserves the Timestep correctly? I.e. if I'd modified the coordinates somehow, this data will be preserved?

yuxuanzhuang · 2022-09-12T09:31:33Z

@richardjgowers Okay, I never thought about preserving the modification of coordinates (before).

But the behavior does change after this PR, i.e. now the modification is preserved (because no data reloading from the file is performed after this line is removed) which is more "correct".

u = mda.Universe(XYZ)

print('Orig pos:,', u.trajectory.ts.positions[0])
u.trajectory.ts.positions[0] = np.array([1, 2, 3])
print('New pos:,', u.trajectory.ts.positions[0])
u_new = pickle.loads(pickle.dumps(u))
print('After serializing pos:,', u_new.trajectory.ts.positions[0])

# original output
Orig pos:, [ 0.931 17.318 16.423]
New pos:, [1. 2. 3.]
After serializing pos:, [ 0.931 17.318 16.423]

# new output after #PR 3722
Orig pos:, [ 0.931 17.318 16.423]
New pos:, [1. 2. 3.]
After serializing pos:, [1. 2. 3.]

richardjgowers · 2022-09-12T10:19:21Z

@yuxuanzhuang thanks for adding the test - it looks like it found that h5md format has a custom serialisation mechanism

yuxuanzhuang · 2022-09-12T11:16:34Z

Should work now! (too many formats to keep track of :) )

orbeckst · 2023-12-15T20:47:35Z

@yuxuanzhuang could you resolve the conflicts please?

orbeckst · 2023-12-15T20:51:08Z

@yuxuanzhuang and @richardjgowers this PR still looks relevant. Why did it stall? What needs to be done to complete it?

github-actions · 2023-12-16T11:40:44Z

Linter Bot Results:

Hi @yuxuanzhuang! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/8540187312/job/23396655061

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

yuxuanzhuang · 2023-12-16T11:42:50Z

@yuxuanzhuang could you resolve the conflicts please?

Done! Should I worry about the linter bot?

IAlibay · 2023-12-16T11:44:37Z

@yuxuanzhuang could you resolve the conflicts please?

Done! Should I worry about the linter bot?

Unless it outlines a flake8 failure it's ok to ignore.

Just checked - no flake8 so you're good to go.

yuxuanzhuang · 2023-12-18T09:25:00Z

I understand why TNGReader needs to re-read the current frame during serialization; however, due to this, the modification of ts cannot be preserved in the TNG format. If I comment it out, all the tests will pass.

        # make sure we re-read the current frame to update C level objects in
        # the file iterator
        self._read_frame(self._frame)

Do you think it's reasonable to only make sure _file_iterator reads the step but no change of ts? @hmacdope

        # convert from frames to integrator steps
        step = self._frame_to_step(self._frame + 1)
        iterator_step = self._file_iterator.read_step(step)

hmacdope · 2023-12-19T22:50:38Z

I understand why TNGReader needs to re-read the current frame during serialization; however, due to this, the modification of ts cannot be preserved in the TNG format. If I comment it out, all the tests will pass.
        # make sure we re-read the current frame to update C level objects in
        # the file iterator
        self._read_frame(self._frame)
Do you think it's reasonable to only make sure _file_iterator reads the step but no change of ts? @hmacdope
        # convert from frames to integrator steps
        step = self._frame_to_step(self._frame + 1)
        iterator_step = self._file_iterator.read_step(step)

Just checking my understanding @yuxuanzhuang, this means that the coordinate data will be re-read, but that any additional data in ts will become invalid? I am just not sure exactly what is meant by change of ts. Perhaps a super quick example that demos the difference?

Sorry about this mess, tbh this is being caused by my lazyness from way back where I didn't make the cython level classes pickleable in pytng (its a bit complicated for __cinnit__ using classes). See MDAnalysis/pytng#97. If I am understanding correctly it would be better to just be able to pickle everything there, rather than forcing a re-read of the step? I can look at addressing this but may take a while as super busy. Perhaps better to go for an interim solution that you suggested above?

hmacdope

Tiny cosmetic things :)

hmacdope · 2023-12-19T23:08:32Z

package/CHANGELOG

@@ -48,6 +49,8 @@ Enhancements
    (Issue #3994, PR #4281)  
  * Add support for reading chainID info from Autodock PDBQT files (Issue #4207,
    PR #4284)
+  * Improve performance in Universe serialization by no explicit redirection to


Newest first :)

hmacdope · 2023-12-19T23:08:44Z

package/CHANGELOG

@@ -198,6 +201,10 @@ Fixes
  * Fix MSD docs to use the correct error metric in example (Issue #3991)
  * Add 'PairIJ Coeffs' to the list of sections in LAMMPSParser.py
    (Issue #3336)
+  * Fix failure in double-serialization of TextIOPicklable file reader.


hmacdope · 2023-12-19T23:08:50Z

package/CHANGELOG

@@ -364,6 +371,8 @@ Changes
    use `pip install ./package[extra_formats]` instead (PR #3810)
  * `Universe.empty` emmits less warnings (PR #3814)
  * adding element attribute to TXYZParser if all atom names are valid element symbols (PR #3826)
+  * TextIOPicklable serializes the raw class instance instead of class name.


orbeckst · 2024-03-30T17:16:59Z

@hmacdope and @richardjgowers can you please check if your raised concerns have been addressed?

If yes, can we merge?

Otherwise, are there bigger issues that we cannot solve?

Forgot to move entries from 2.7 to 2.8 in merge

moved an entry from 2.4.1(!) to 2.8.0

orbeckst

update versionchanged to 2.8.0

Note that I had to find the CHANGELOG entries from different sections to correctly merge. Please double check the CHANGELOG @yuxuanzhuang

package/MDAnalysis/coordinates/base.py

package/MDAnalysis/lib/picklable_file_io.py

yuxuanzhuang · 2024-04-03T15:00:14Z

I am just not sure exactly what is meant by change of ts. Perhaps a super quick example that demos the difference?

I mean modification of e.g. coordinates.

u = mda.Universe(TNG_traj_gro, TNG_traj)
reader = u.trajectory
reader.ts.positions[0] = np.array([1, 2, 3])
reader_p = pickle.loads(pickle.dumps(reader))
assert_equal(reader.ts, reader_p.ts,
                       "Modification of ts not preserved after serialization")

Previously, during unpickling, self._read_frame(self._frame) will 1) update self._file_iterator to the current frame. 2) modify self.ts by reading from self._file_iterator. This will discard any change to the timestep itself.

Now, only self._file_iterator will be reopened and updated. self.ts will be preserved during serialization.

hmacdope

LGTM @yuxuanzhuang, thanks for pushing through on this. 👍

orbeckst · 2024-04-04T00:44:22Z

@richardjgowers is your review still pertinent or should I just dismiss it?

serialize without visit ts

6a667f6

github-actions bot added Component-lib Component-Readers labels Jun 19, 2022

yuxuanzhuang changed the title ~~Not redirect to the original frame during deserialization (WIP)~~ No need to redirect to the original frame explicitly during deserialization (WIP) Jun 19, 2022

add double pickling test

a8a5485

yuxuanzhuang mentioned this pull request Jun 19, 2022

Improve Universe serialization performance #3721

Closed

yuxuanzhuang added 2 commits June 20, 2022 00:02

offset should be the same

b358bd2

add comment about bufferio

5b035fc

yuxuanzhuang marked this pull request as draft June 23, 2022 09:47

richardjgowers requested changes Jun 29, 2022

View reviewed changes

doc

b186584

merge to dev

b1a3770

yuxuanzhuang marked this pull request as ready for review June 30, 2022 12:06

merge to dev

8fda4ef

richardjgowers reviewed Sep 12, 2022

View reviewed changes

yuxuanzhuang changed the title ~~No need to redirect to the original frame explicitly during deserialization (WIP)~~ No need to redirect to the original frame explicitly during deserialization Sep 12, 2022

yuxuanzhuang added 3 commits September 12, 2022 11:32

Merge remote-tracking branch 'mda_origin/develop' into opt_serialization

bef41aa

add changelog

917dc65

add test for preservation of coord after serialization

bd553c1

yuxuanzhuang added 2 commits September 12, 2022 12:58

fix h5md setstate

884a795

fix h5md comments

8535692

yuxuanzhuang added 2 commits January 20, 2023 13:55

merge to dev

bce27a3

merge to dev

8a1222a

orbeckst self-assigned this Dec 15, 2023

merge to dev

dc40dd6

hmacdope requested changes Dec 19, 2023

View reviewed changes

orbeckst added 3 commits March 30, 2024 10:20

Merge branch 'develop' into opt_serialization

35354be

fixed CHANGELOG

f6d5716

Forgot to move entries from 2.7 to 2.8 in merge

more CHANGELOG fixes

5b6b521

moved an entry from 2.4.1(!) to 2.8.0

orbeckst reviewed Mar 30, 2024

View reviewed changes

package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved

package/MDAnalysis/lib/picklable_file_io.py Outdated Show resolved Hide resolved

orbeckst and others added 4 commits March 30, 2024 10:29

Apply suggestions from code review

f3cc8d3

tng setstate

845dede

Merge branch 'develop' into opt_serialization

ff31fc9

pep8

1a6272e

hmacdope approved these changes Apr 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No need to redirect to the original frame explicitly during deserialization #3722

No need to redirect to the original frame explicitly during deserialization #3722

yuxuanzhuang commented Jun 19, 2022 •

edited

pep8speaks commented Jun 19, 2022 •

edited

codecov bot commented Jun 19, 2022 •

edited

orbeckst commented Jun 22, 2022

richardjgowers left a comment

yuxuanzhuang commented Jun 30, 2022

richardjgowers left a comment

yuxuanzhuang commented Sep 12, 2022

richardjgowers commented Sep 12, 2022

yuxuanzhuang commented Sep 12, 2022

orbeckst commented Dec 15, 2023

orbeckst commented Dec 15, 2023

github-actions bot commented Dec 16, 2023 •

edited

yuxuanzhuang commented Dec 16, 2023

IAlibay commented Dec 16, 2023 •

edited

yuxuanzhuang commented Dec 18, 2023

hmacdope commented Dec 19, 2023

hmacdope left a comment

hmacdope Dec 19, 2023

hmacdope Dec 19, 2023

hmacdope Dec 19, 2023

orbeckst commented Mar 30, 2024

orbeckst left a comment

yuxuanzhuang commented Apr 3, 2024

hmacdope left a comment

orbeckst commented Apr 4, 2024

No need to redirect to the original frame explicitly during deserialization #3722

Are you sure you want to change the base?

No need to redirect to the original frame explicitly during deserialization #3722

Conversation

yuxuanzhuang commented Jun 19, 2022 • edited

PR Checklist

pep8speaks commented Jun 19, 2022 • edited

Comment last updated at 2024-04-03 14:10:03 UTC

codecov bot commented Jun 19, 2022 • edited

Codecov Report

orbeckst commented Jun 22, 2022

richardjgowers left a comment

Choose a reason for hiding this comment

yuxuanzhuang commented Jun 30, 2022

richardjgowers left a comment

Choose a reason for hiding this comment

yuxuanzhuang commented Sep 12, 2022

richardjgowers commented Sep 12, 2022

yuxuanzhuang commented Sep 12, 2022

orbeckst commented Dec 15, 2023

orbeckst commented Dec 15, 2023

github-actions bot commented Dec 16, 2023 • edited

Linter Bot Results:

yuxuanzhuang commented Dec 16, 2023

IAlibay commented Dec 16, 2023 • edited

yuxuanzhuang commented Dec 18, 2023

hmacdope commented Dec 19, 2023

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope Dec 19, 2023

Choose a reason for hiding this comment

hmacdope Dec 19, 2023

Choose a reason for hiding this comment

hmacdope Dec 19, 2023

Choose a reason for hiding this comment

orbeckst commented Mar 30, 2024

orbeckst left a comment

Choose a reason for hiding this comment

yuxuanzhuang commented Apr 3, 2024

hmacdope left a comment

Choose a reason for hiding this comment

orbeckst commented Apr 4, 2024

yuxuanzhuang commented Jun 19, 2022 •

edited

pep8speaks commented Jun 19, 2022 •

edited

codecov bot commented Jun 19, 2022 •

edited

github-actions bot commented Dec 16, 2023 •

edited

IAlibay commented Dec 16, 2023 •

edited