Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Serialize FileIO and TextIOWrapper and Universe #2723
Serialize FileIO and TextIOWrapper and Universe #2723
Changes from 11 commits
931d9b5
d69ce98
432edf3
bf55bf3
cd9a485
f33629f
aca4496
cd4ffe3
ec5bd3c
f6515ee
cf29764
6b09e20
c29c316
db47e27
7e9d6d3
eb83a7c
0baa868
c8e63b2
658b446
1aa6003
f2738bc
001f3b8
d0374f5
8c62df8
94f1f8d
acbadec
7043d2d
546b05d
c259143
a016a65
401e6ae
9225c71
46c43af
21fe5aa
821c822
5e25380
2541a3e
1003cd3
1b7a798
b79d282
d909d63
cafc596
2db1ef2
33ef68a
24f2a34
84baca9
108ebde
7cb40ad
352ab96
356986f
e5ef732
aa6e40d
43a62d5
2559625
507f8f5
26fcfe9
405a6dc
2380a47
dab38c1
5c07901
b324791
49f959d
773524d
b7e4ef0
9d376b7
11cceb4
a3130f5
faf1e01
df7eb86
72ba276
aa62ff0
04be63d
f01769f
5a2b28d
b5f5270
e1facfb
cba4456
5622b51
46cda48
5e2ee79
b23b2fb
cd03058
3ce8ba7
a5da2f7
5a9ad4d
bc60aa7
01fc644
84eb61f
9f18ccd
8d07004
e37c84a
2d3de99
67b65d1
18d146b
8679e50
0ceffe5
688041c
204545b
3c71f8a
f2239bb
78c93a0
c0d241e
68b1c2a
4061434
d457491
4c70dcb
0496ca1
df061fc
abe92da
b3469fe
b12eb0d
fae4797
52a981e
c4ec287
8804e5b
c99867f
a70bc8b
bc487a5
a1bb47e
5ace1e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should fail for anything else (
ValueError
for unsupportedmode
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment here saying that we should never get here; it's not obvious until you read all code up to this point.
Or better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean replacing the
ValueError
for unsupported mode?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you already test that files can be pickled that is enough as multiprocessing is using pickles internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm not sure how relevant this is to how we use file objects, but I know that you can get f.tell() to be "disabled" if you happened to call f.next() (I think it only happens if you only partially iterate through the file). Might be worth keeping in mind as a potential failure point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true! I already noticed this failure for gms, mdcdr formats. I think in the context of pickling trajectories, there's not much use (?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know of a failure point then we should test for it – if nothing else test that it raises an exception as we expect. At least then we know how to reproduce the problem when we need to look closer into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I added a test to raise this error. Maybe in the future, we can either:
f.tell()
fails.