Skip to content
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

Cosmo rename arg for io #14780

Merged
merged 5 commits into from Jun 20, 2023
Merged

Cosmo rename arg for io #14780

merged 5 commits into from Jun 20, 2023

Conversation

nstarman
Copy link
Member

@nstarman nstarman commented May 6, 2023

Adding an option to remap symbols in input and output of a cosmology.
This should be helpful for #14701.

@nstarman nstarman added this to the v6.0 milestone May 6, 2023
@github-actions
Copy link

github-actions bot commented May 6, 2023

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions
Copy link

github-actions bot commented May 6, 2023

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@nstarman nstarman marked this pull request as ready for review May 6, 2023 20:44
@nstarman nstarman requested a review from a team as a code owner May 6, 2023 20:44
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

astropy/cosmology/io/table.py Outdated Show resolved Hide resolved
astropy/cosmology/io/table.py Outdated Show resolved Hide resolved
astropy/cosmology/io/table.py Outdated Show resolved Hide resolved
astropy/cosmology/io/mapping.py Outdated Show resolved Hide resolved
@pllim
Copy link
Member

pllim commented May 7, 2023

but found the large number of unrelated code and docstring changes distracting

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.

@nstarman
Copy link
Member Author

nstarman commented May 7, 2023

Overall, I like the idea but found the large number of unrelated code and docstring changes distracting and ended up running out of time.

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.

@eerovaher
Copy link
Member

@mhvk wrote:

...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.

I also think renaming things at the astropy.io level could be useful. For example, I would find it convenient if I could work in Python with a Table with columns named "ra" and "dec", but when I write a LaTeX file using astropy.io I might want the column names to appear as "$\alpha$" and "$\delta$". So if there is a way of implementing renaming things so that both astropy.table and astropy.cosmology would benefit then that should be considered. I am not familiar enough with astropy.io to know if this would be simple to achieve or not, so it would be good to know what @taldcroft thinks.

@nstarman nstarman force-pushed the cosmo-rename-params branch 2 times, most recently from 5fe2232 to 9d45814 Compare May 8, 2023 19:26
@nstarman nstarman requested a review from mhvk May 17, 2023 22:29
@nstarman
Copy link
Member Author

@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.

@nstarman nstarman force-pushed the cosmo-rename-params branch 2 times, most recently from 91c23b5 to 533e8b2 Compare May 17, 2023 22:45
@nstarman nstarman changed the title Cosmo rename arg to io Cosmo rename arg for io May 20, 2023
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is a good idea, but I do have some suggestions regarding the implementation.

astropy/cosmology/io/mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/row.py Outdated Show resolved Hide resolved
astropy/cosmology/io/tests/test_mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/tests/test_mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/tests/test_mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/tests/test_mapping.py Outdated Show resolved Hide resolved
astropy/cosmology/io/ecsv.py Outdated Show resolved Hide resolved
@nstarman
Copy link
Member Author

Thanks @eerovaher for the detailed review!

@nstarman nstarman force-pushed the cosmo-rename-params branch 2 times, most recently from 0daa36c to efd22ff Compare May 23, 2023 14:35
@nstarman
Copy link
Member Author

I squashed commits, added and modified tests, added to the Examples in the docstring, and put in a "whatsnew".

@nstarman nstarman requested a review from eerovaher June 10, 2023 17:34
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

astropy/cosmology/io/row.py Outdated Show resolved Hide resolved
docs/whatsnew/6.0.rst Show resolved Hide resolved
docs/whatsnew/6.0.rst Show resolved Hide resolved
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few small requests regarding the documentation. I think we can merge when they are addressed.

docs/cosmology/io.rst Outdated Show resolved Hide resolved
docs/whatsnew/6.0.rst Outdated Show resolved Hide resolved
docs/whatsnew/6.0.rst Outdated Show resolved Hide resolved
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nstarman
Copy link
Member Author

Yikes, but failures are unrelated.

@pllim
Copy link
Member

pllim commented Jun 17, 2023

numpy 1.25 is released but h5py didn't catch up. Annoying...

@eerovaher
Copy link
Member

The deprecation warnings caused by h5py have been dealt with on main, so I suggest rebasing to allow all the CI tests to succeed.

nstarman and others added 5 commits June 19, 2023 16:31
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
@eerovaher
Copy link
Member

I'd like to merge, but the change log entry CI check is stuck. @pllim, what's the best way to proceed?

@pllim pllim merged commit e9ef294 into astropy:main Jun 20, 2023
23 of 24 checks passed
@pllim
Copy link
Member

pllim commented Jun 20, 2023

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.

@pllim
Copy link
Member

pllim commented Jun 20, 2023

Thanks, all!

@mhvk
Copy link
Contributor

mhvk commented Jun 20, 2023

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.

@pllim
Copy link
Member

pllim commented Jun 20, 2023

The problem here is we rely on Actions, so it competes with other jobs and sometimes GitHub might flag it as spammy.

@nstarman nstarman deleted the cosmo-rename-params branch June 20, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants