Added copy method for AuxReaders #3887

BFedder · 2022-10-27T15:10:26Z

Fixes #1785

Changes made in this Pull Request:

Added a copy method for AuxReaders to make them and Universes that have them copyable.
Added tests to make sure all currently implemented AuxReaders (XVG, XVG-File, EDR) are copied properly

PR Checklist

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

codecov · 2022-10-27T15:35:38Z

Codecov Report

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

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

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3887   +/-   ##
========================================
  Coverage    93.52%   93.52%           
========================================
  Files          190      190           
  Lines        25028    25037    +9     
  Branches      3542     3543    +1     
========================================
+ Hits         23407    23417   +10     
+ Misses        1100     1099    -1     
  Partials       521      521

Impacted Files	Coverage Δ
package/MDAnalysis/auxiliary/base.py	`92.04% <100.00%> (+0.11%)`	⬆️
MDAnalysisTests/auxiliary/base.py	`89.85% <0.00%> (+0.11%)`	⬆️
MDAnalysisTests/coordinates/base.py	`94.63% <0.00%> (+0.17%)`	⬆️

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.

richardjgowers · 2022-10-29T10:05:48Z

package/MDAnalysis/auxiliary/base.py

@@ -323,7 +323,9 @@ def __init__(self, represent_ts_as='closest', auxname=None, cutoff=None,
            self.rewind()

    def copy(self):


in the docstring can it be made explicit if this is a deep (a completely independent thing with its own file handles etc, no shared resources) or shallow (might share a resource like a file handle with the original) copy?

I'm going to block on this query for docs from @richardjgowers - also changelog, etc.. the usuals

I think "deep" was meant to clarify

Missed that, but please do take the above as an overall "docs" please - versionchanged addition would be good

Yeah sorry I approved too hastily my brain isn't working properly today. 😅

richardjgowers · 2022-10-29T10:06:50Z

@BFedder this looks good. With how you're copying the ould it make more sense to define __getstate__ and __setstate__ so that AuxReader will serialise, then the copying can be done by serialising/deserialising in the same function and returning that? Two birds with one stone.

BFedder · 2022-11-03T16:55:22Z

I have never used __getstate__ and __setstate__ before, so I'm not quite sure... Were you thinking of something like this?

hmacdope

This looks good to me @BFedder

hmacdope · 2022-11-12T00:44:02Z

package/MDAnalysis/auxiliary/base.py

+        new_reader = pickle.loads(orig_dict)
+        return new_reader
+
+    def __getstate__(self):


Wait @BFedder quick query does the pickling work without __getstate__ and __setstate__ defined here? IIRC pickle does have a default that is very similar to this that does not require the methods to be explicitly defined.
See here

IAlibay · 2022-11-12T00:47:02Z

package/MDAnalysis/auxiliary/base.py

@@ -323,7 +323,9 @@ def __init__(self, represent_ts_as='closest', auxname=None, cutoff=None,
            self.rewind()

    def copy(self):


I'm going to block on this query for docs from @richardjgowers - also changelog, etc.. the usuals

hmacdope · 2022-11-12T00:49:49Z

@IAlibay sorry forgot CHANGELOG.

BFedder · 2022-11-21T16:45:37Z

Added versionchanged and changelog entries now. Also, yes @hmacdope, turns out the __getstate__ and __setstate__ definitions were redundant - thanks!

orbeckst · 2023-01-06T22:27:54Z

@BFedder would you mind resolving conflicts?

@richardjgowers / @IAlibay can you please look if you're happy or state what needs to be done to complete the PR?

IAlibay

I went ahead and fixed the merge issue with the changelog, lgtm!

BFedder · 2023-01-07T10:34:19Z

Thanks for the fix, @IAlibay!

hmacdope

LGTM

orbeckst · 2023-02-13T03:24:07Z

@BFedder could you please update the PR and then we can merge it. Sorry for the delay, not sure why we didn't merge more than a month ago. If in doubt, just ping people after a few days...

orbeckst · 2023-02-13T03:25:38Z

darker is complaining about quotation marks ... https://github.com/MDAnalysis/mdanalysis/actions/runs/3859059696/jobs/6578235368 — if you could please fix those, too, then that's one thing less to worry about later

BFedder · 2023-02-13T11:29:31Z

I have taken care of the merge conflicts and the quotation marks darker was complaining about, but now the azure run for Win-Python38-32 bit is failing because test_write_trajectory_universe in coordinates/test_h5md.py failed. I'm not sure why this is happening in this setup in particular and nowhere else. Could I please ask for advice on how to fix this?

orbeckst · 2023-02-13T21:02:58Z

https://stackoverflow.com/questions/49151057/valueerror-not-a-location-id-invalid-object-id-while-creating-hdf5-datasets suggests that our error

____________ TestH5MDWriterBaseAPI.test_write_trajectory_universe _____________
[gw0] win32 -- Python 3.8.10 C:\hostedtoolcache\windows\Python\3.8.10\x86\python.exe

self = <MDAnalysisTests.coordinates.test_h5md.TestH5MDWriterBaseAPI object at 0x0D0DCF88>
ref = <MDAnalysisTests.coordinates.test_h5md.H5MDReference object at 0x144076D0>
reader = <H5MDReader D:\a\1\s\testsuite\MDAnalysisTests\data\coordinates\test.h5md with 5 frames of 5 atoms>
universe = <Universe with 5 atoms>
tmpdir = local('C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\pytest-of-VssAdministrator\\pytest-0\\popen-gw0\\test_write_trajectory_universe7')

    def test_write_trajectory_universe(self, ref, reader, universe, tmpdir):
        outfile = 'write-uni-test.' + ref.ext
        with tmpdir.as_cwd():
            with ref.writer(outfile, universe.atoms.n_atoms,
                            velocities=True, forces=True) as w:
>               for ts in universe.trajectory:

D:\a\1\s\testsuite\MDAnalysisTests\coordinates\test_h5md.py:135: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\hostedtoolcache\windows\Python\3.8.10\x86\lib\site-packages\mdanalysis-2.5.0.dev0-py3.8-win32.egg\MDAnalysis\coordinates\base.py:795: in __iter__
    self._reopen()
C:\hostedtoolcache\windows\Python\3.8.10\x86\lib\site-packages\mdanalysis-2.5.0.dev0-py3.8-win32.egg\MDAnalysis\coordinates\H5MD.py:747: in _reopen
    self.close()
C:\hostedtoolcache\windows\Python\3.8.10\x86\lib\site-packages\mdanalysis-2.5.0.dev0-py3.8-win32.egg\MDAnalysis\coordinates\H5MD.py:728: in close
    self._file.close()
C:\hostedtoolcache\windows\Python\3.8.10\x86\lib\site-packages\h5py\_hl\files.py:435: in close
    file_list = [x for x in file_list if h5i.get_file_id(x).id == self.id.id]
>   ???
E   ValueError: Not an id of a file object (not an ID of a file object)

h5py\h5i.pyx:114: ValueError

is due to access on a closed file.

This seems odd as the same tests pass elsewhere.

I'll try to restart the test (and send a few prayers or curses in the general direction of the code deities).

BFedder · 2023-02-13T22:42:56Z

Your prayers and/or curses appear to have done the trick, thanks! How odd. Is this ready to merge, then?

Added copy method for AuxReaders

3c4d55e

github-actions bot added the Component-Readers label Oct 27, 2022

reverted unnecessary change

01f6b0f

github-actions bot removed the Component-Readers label Oct 27, 2022

pyedr is optional, account for in test

3f6aee2

richardjgowers reviewed Oct 29, 2022

View reviewed changes

BFedder added 2 commits November 3, 2022 16:52

Using __getstate__ and __setstate__

210caff

made clear it's a deep copy

206b0b8

hmacdope approved these changes Nov 12, 2022

View reviewed changes

hmacdope requested changes Nov 12, 2022

View reviewed changes

IAlibay requested changes Nov 12, 2022

View reviewed changes

BFedder and others added 2 commits November 21, 2022 16:41

removed setstate and getstate methods, added versionchanged, changelog

d00e3e1

Merge branch 'develop' into aux_copy

4d36da1

Merge branch 'develop' into aux_copy

c0a389e

IAlibay approved these changes Jan 6, 2023

View reviewed changes

hmacdope approved these changes Jan 7, 2023

View reviewed changes

BFedder added 4 commits February 13, 2023 09:43

Merge branch 'develop' into aux_copy

7bd0610

Update test_copying.py

b1eefd8

Update base.py

009f592

Update test_copying.py

5492083

orbeckst merged commit 94904b5 into MDAnalysis:develop Feb 13, 2023

IAlibay added the enhancement label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added copy method for AuxReaders #3887

Added copy method for AuxReaders #3887

BFedder commented Oct 27, 2022

codecov bot commented Oct 27, 2022 •

edited

Loading

richardjgowers Oct 29, 2022

IAlibay Nov 12, 2022

hmacdope Nov 12, 2022

IAlibay Nov 12, 2022

hmacdope Nov 12, 2022

richardjgowers commented Oct 29, 2022

BFedder commented Nov 3, 2022

hmacdope left a comment

hmacdope Nov 12, 2022

IAlibay Nov 12, 2022

hmacdope commented Nov 12, 2022

BFedder commented Nov 21, 2022

orbeckst commented Jan 6, 2023

IAlibay left a comment

BFedder commented Jan 7, 2023

hmacdope left a comment

orbeckst commented Feb 13, 2023

orbeckst commented Feb 13, 2023

BFedder commented Feb 13, 2023

orbeckst commented Feb 13, 2023

BFedder commented Feb 13, 2023

		@@ -323,7 +323,9 @@ def __init__(self, represent_ts_as='closest', auxname=None, cutoff=None,
		self.rewind()

		def copy(self):

Added copy method for AuxReaders #3887

Added copy method for AuxReaders #3887

Conversation

BFedder commented Oct 27, 2022

PR Checklist

codecov bot commented Oct 27, 2022 • edited Loading

Codecov Report

richardjgowers Oct 29, 2022

Choose a reason for hiding this comment

IAlibay Nov 12, 2022

Choose a reason for hiding this comment

hmacdope Nov 12, 2022

Choose a reason for hiding this comment

IAlibay Nov 12, 2022

Choose a reason for hiding this comment

hmacdope Nov 12, 2022

Choose a reason for hiding this comment

richardjgowers commented Oct 29, 2022

BFedder commented Nov 3, 2022

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope Nov 12, 2022

Choose a reason for hiding this comment

IAlibay Nov 12, 2022

Choose a reason for hiding this comment

hmacdope commented Nov 12, 2022

BFedder commented Nov 21, 2022

orbeckst commented Jan 6, 2023

IAlibay left a comment

Choose a reason for hiding this comment

BFedder commented Jan 7, 2023

hmacdope left a comment

Choose a reason for hiding this comment

orbeckst commented Feb 13, 2023

orbeckst commented Feb 13, 2023

BFedder commented Feb 13, 2023

orbeckst commented Feb 13, 2023

BFedder commented Feb 13, 2023

codecov bot commented Oct 27, 2022 •

edited

Loading