Repository navigation
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
There was a problem hiding this comment.
Thanks for the quick fix. I had not even looked into why init_from_class was in the non-byteorder-preserving group, but having that fixed as well is of course yet better!
I have not found any pickle wrappers within our code, so this can indeed be considered a testing-only issue – there is one fixture relevant in
astropy/astropy/tests/helper.py
Line 129 in 76354c8
so it might not hurt to extend this to
params=[0, 1, 4, -1].
Still could be useful to note the different protocols in the docs, though again the only mention of pickle is in cosmology.io. Could suggest there to use pickle.dump(Planck18, file, protocol=5), although the format is most definitely not recommended for exchanging files between different machines, much less different architectures – @nstarman what do you think?
|
I think that's a good idea. |
|
The more reason perhaps to avoid any byteswapping where possible, although I have not benchmarked the two protocols |
1134ec8 to
c394946
Compare
|
Thank ! I updated the PR with your suggestions. |
github-project-automation
Bot
moved this from Triage
to Reviewer approved
in Cosmology, the Expansion
|
Since this is marked as a bug and is going to land on v7.1.0 if backported, it should have a change log. |
|
@dhomeier what do you think ? Do you expect anyone is using this internal API downstream ? |
dhomeier
left a comment
dhomeier
left a comment
There was a problem hiding this comment.
I don't really know how "private" the test helper functions are considered, but in any case the "most advanced" protocol included there has already changed once (4->5 with Python 3.8) since this fixture was introduced in #1947, so downstream users already should have noticed (or never cared; I was hardly aware there are different pickle protocols at all). I think this deserves a bit more extensive documentation and should indeed get a changelog entry as well.
dhomeier
left a comment
dhomeier
left a comment
There was a problem hiding this comment.
Thanks for the formatting help; looks good now, but I think this deserves a test on the big-endian machine!
e491aad to
b60eb34
Compare
| def pickle_protocol(request): | ||
| """ | ||
| Fixture to run all the tests for protocols 0 and 1, and -1 (most advanced). | ||
| Fixture to run all the tests for protocols 0, 1, 4 and 5 (most advanced). |
There was a problem hiding this comment.
Since "5" is already mentioned in bullet point below under "-1", I think we should change this back to "-1" to be less confusing.
| Fixture to run all the tests for protocols 0, 1, 4 and 5 (most advanced). | |
| Fixture to run all the tests for protocols 0, 1, 4 and -1 (most advanced). |
b60eb34 to
3c7acf8
Compare
github-project-automation
Bot
moved this from Reviewer approved
to Done
in Cosmology, the Expansion
…eserve dtype byteorder)
|
Thanks all; should have referred to "-1" in the bullet point right away! |
…dtype byteorder) (#18156) Co-authored-by: Clément Robert <[email protected]>
|
I found that this wasn't actually included in the 7.1.0 release, so, updating the milestone now. |
…undtrips (preserve dtype byteorder))
…undtrips (preserve dtype byteorder))
Backport PR #18139 on `v7.1.x` (TST: improve `ArrayWrapper` roundtrips (preserve dtype byteorder))
Description
Fixes #18127. The primary goal was to fix compatibility of a test with CPython 3.14, but in discussing the root issues we found ways to simplify the test for every supported versions instead.
I'm assuming that
astropy.table.table_helperisn't public API, in which case this can be treated as a dev-only bugfix to the test suite and be backported.follow up to #11545