Added copy method for AuxReaders by BFedder · Pull Request #3887 · MDAnalysis/mdanalysis

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

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

		def copy(self):

Conversation

BFedder commented Oct 27, 2022

PR Checklist

Uh oh!

codecov bot commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richardjgowers Oct 29, 2022

Choose a reason for hiding this comment

Uh oh!

IAlibay Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

hmacdope Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

IAlibay Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

hmacdope Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Oct 29, 2022

Uh oh!

BFedder commented Nov 3, 2022

Uh oh!

hmacdope left a comment

Choose a reason for hiding this comment

Uh oh!

hmacdope Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

IAlibay Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

hmacdope commented Nov 12, 2022

Uh oh!

BFedder commented Nov 21, 2022

Uh oh!

orbeckst commented Jan 6, 2023

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

BFedder commented Jan 7, 2023

Uh oh!

hmacdope left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Feb 13, 2023

Uh oh!

orbeckst commented Feb 13, 2023

Uh oh!

BFedder commented Feb 13, 2023

Uh oh!

orbeckst commented Feb 13, 2023

Uh oh!

BFedder commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Oct 27, 2022 •

edited

Loading