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
Add simple pickle compatibility to ReferenceFrame #25102
Conversation
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Click here to see the pull request description that was parsed.
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[2c3de5f4] [6696dea6]
- 84.0±1ms 54.6±0.4ms 0.65 integrate.TimeIntegrationRisch02.time_doit(10)
Full benchmark results can be found as artifacts in GitHub Actions |
A_C_N = A.dcm(N) | ||
N1 = pickle.loads(pickle.dumps(N)) | ||
A1 = tuple(N1._dcm_dict.keys())[0] | ||
assert A1.dcm(N1) == A_C_N |
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.
The assertion only checks that these two matrices are the same but you pickle the reference frame. Shouldn't you be checking reference frames before and after?
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 currently check whether the system still behaves the same. N1 actually won't equal N, as the hash is based on the memory location, which is not the same.
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.
ok, i don't think this hurts anything, so let's merge
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.
An option I actually looked into was using self._hash = super().__hash__()
as hash value, which is set once. However this will actually not be directly compatible with pickling, due to the bidirectional relationships between frames. A solution would be to have pickle.load
(a __setstate__
method) first construct all the frames with their hashes and only then define the relationships between them. If this is really a strong preference, I can do something like that, which also needs to happen in Point
. Problem is that this would just introduce a lot of complexity for a rather unnecessary feature in my opinion.
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.
At the minimum you could open an issue or add to one of the existing issues an explanation of the further complexity.
Thanks for merging |
References to other Issues or PRs
Partially fixes #279 from dill.
Brief description of what is fixed or changed
Implement a
__getnewargs_ex__
inCoordinateSym
to enable some simple pickling of reference frames. One should however note that aReferenceFrame
is not exactly the same after de-serialization as before, due to the hash being based on the memory location. Another problem is thatdynamicsymbols
are not pickable, as described in #4297 (this last problem seems to be fixed incloudpickle
). An option I looked into for solving the hash problem, namely creating a_hash
attribute did not work well, due to having a bi-directional graph that uses dictionaries (and so the hashes).Other comments
Release Notes
NO ENTRY