Repository navigation
Cosmo rename arg for io - #14780
Cosmo rename arg for io#14780
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 |
c3e4cd8 to
5e7ab10
Compare
mhvk
left a comment
There was a problem hiding this comment.
@nstarman - Overall, I like the idea but found the large number of unrelated code and docstring changes distracting and ended up running out of time. So, mostly a few grumpy comments inline, plus a slightly larger question: don't we have something like this already in the general IO? I tried looking and couldn't find it, so probably not, but let me ping @taldcroft regardless, since perhaps this option to rename things would be something we should support more generally.
My mai
I did a drive-by glance but gave up for the same reason too. It was unclear to me why these are necessary for the renaming process. |
Apologies for the constellation of changes surrounding the main portion of the PR. These changes arose because when I added the docstring for the new argument I wanted to make sure I got the format correct, following numpydoc to the T. I found that the docstring was incorrectly implemented 😲 and rather than mixing styles within one docstring, I fixed the relevant docstrings. |
5e7ab10 to
e357ed3
Compare
dec63c7 to
7cf046c
Compare
|
@mhvk wrote:
I also think renaming things at the |
5fe2232 to
9d45814
Compare
|
@eerovaher, I agree, but m any of the Cosmology I/O methods do not rely on Table, so lacking prior art from table, I think this PR is a good place to start. |
91c23b5 to
533e8b2
Compare
eerovaher
left a comment
There was a problem hiding this comment.
Overall I think this is a good idea, but I do have some suggestions regarding the implementation.
|
Thanks @eerovaher for the detailed review! |
0daa36c to
efd22ff
Compare
|
I squashed commits, added and modified tests, added to the Examples in the docstring, and put in a "whatsnew". |
eerovaher
left a comment
There was a problem hiding this comment.
I have just a few remarks regarding the documentation in this pull request. But I should also point out that the astropy.cosmology I/O documentation isn't being edited at all.
bbc70ee to
ea06f1e
Compare
eerovaher
left a comment
There was a problem hiding this comment.
I'm happy with all the changes this pull request currently makes, but the new feature still has to be documented in astropy.cosmology I/O documentation.
ea06f1e to
1fd38bf
Compare
eerovaher
left a comment
There was a problem hiding this comment.
I have a few small requests regarding the documentation. I think we can merge when they are addressed.
69d7161 to
5a285af
Compare
eerovaher
left a comment
There was a problem hiding this comment.
Merging the pull request that renamed astropy.cosmology.io to astropy.cosmology._io has created merge conflicts that need to be solved, but other than that this looks good.
5a285af to
cbdda82
Compare
|
Yikes, but failures are unrelated. |
|
numpy 1.25 is released but h5py didn't catch up. Annoying... |
|
The deprecation warnings caused by |
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]> Co-authored-by: Eero Vaher <[email protected]>
cbdda82 to
fd0c44f
Compare
|
I'd like to merge, but the change log entry CI check is stuck. @pllim, what's the best way to proceed? |
|
Best way was probably what you did. I merged it. 😉 Sigh, if only there is a better way to run change log check... it gets stuck more and more often now. |
|
Thanks, all! |
|
I usually just turn labels on and off so it runs again (and nothing else does), but it is indeed a bit of a pain. |
|
The problem here is we rely on Actions, so it competes with other jobs and sometimes GitHub might flag it as spammy. |
Adding an option to remap symbols in input and output of a cosmology.
This should be helpful for #14701.