Skip to content

Cosmo rename arg for io - #14780

Merged
pllim merged 5 commits into
astropy:mainfrom
nstarman:cosmo-rename-params
Jun 20, 2023
Merged

pllim merged 5 commits into
astropy:mainfrom
nstarman:cosmo-rename-params

Conversation

@nstarman

@nstarman nstarman commented May 6, 2023 •

Copy link
Copy Markdown
Member

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

github-actions Bot commented May 6, 2023 •

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented May 6, 2023

Copy link
Copy Markdown
Contributor

👋 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 force-pushed the cosmo-rename-params branch from c3e4cd8 to 5e7ab10 Compare May 6, 2023 20:44
@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

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread astropy/cosmology/io/table.py Resolved
Comment thread astropy/cosmology/io/table.py Resolved
Comment thread astropy/cosmology/io/table.py Resolved
Comment thread astropy/cosmology/io/mapping.py Resolved
@pllim

pllim commented May 7, 2023

Copy link
Copy Markdown
Member

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

nstarman commented May 7, 2023 •

Copy link
Copy Markdown
Member Author

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.

@nstarman
nstarman force-pushed the cosmo-rename-params branch from 5e7ab10 to e357ed3 Compare May 7, 2023 15:39
@nstarman
nstarman force-pushed the cosmo-rename-params branch 2 times, most recently from dec63c7 to 7cf046c Compare May 8, 2023 19:08
@eerovaher

Copy link
Copy Markdown
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
Copy Markdown
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

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

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.

Comment thread astropy/cosmology/io/mapping.py Resolved
Comment thread astropy/cosmology/io/mapping.py Resolved
Comment thread astropy/cosmology/io/mapping.py Resolved
Comment thread astropy/cosmology/io/mapping.py Resolved
Comment thread astropy/cosmology/io/row.py Resolved
Comment thread astropy/cosmology/io/ecsv.py Resolved
@nstarman

Copy link
Copy Markdown
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
Comment thread astropy/cosmology/io/mapping.py Resolved
@nstarman

Copy link
Copy Markdown
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

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

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.

Comment thread astropy/cosmology/io/row.py Resolved
Comment thread docs/whatsnew/6.0.rst Resolved
Comment thread docs/whatsnew/6.0.rst Resolved
@nstarman
nstarman force-pushed the cosmo-rename-params branch from bbc70ee to ea06f1e Compare June 15, 2023 14:12

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

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.

@nstarman
nstarman force-pushed the cosmo-rename-params branch from ea06f1e to 1fd38bf Compare June 16, 2023 14:10
@nstarman
nstarman requested a review from eerovaher June 16, 2023 15:25

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

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.

Comment thread docs/cosmology/io.rst Resolved
Comment thread docs/whatsnew/6.0.rst Resolved
Comment thread docs/whatsnew/6.0.rst Resolved
@nstarman
nstarman force-pushed the cosmo-rename-params branch from 69d7161 to 5a285af Compare June 16, 2023 19:29
@nstarman
nstarman requested a review from eerovaher June 16, 2023 19:29

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

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
nstarman force-pushed the cosmo-rename-params branch from 5a285af to cbdda82 Compare June 17, 2023 18:19
@nstarman
nstarman requested a review from eerovaher June 17, 2023 18:21
@nstarman

Copy link
Copy Markdown
Member Author

Yikes, but failures are unrelated.

@pllim

pllim commented Jun 17, 2023

Copy link
Copy Markdown
Member

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

@eerovaher

Copy link
Copy Markdown
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 <[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]>
@nstarman
nstarman force-pushed the cosmo-rename-params branch from cbdda82 to fd0c44f Compare June 19, 2023 20:32
@eerovaher

Copy link
Copy Markdown
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
@pllim

pllim commented Jun 20, 2023

Copy link
Copy Markdown
Member

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

pllim commented Jun 20, 2023

Copy link
Copy Markdown
Member

Thanks, all!

@mhvk

mhvk commented Jun 20, 2023

Copy link
Copy Markdown
Contributor

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

pllim commented Jun 20, 2023

Copy link
Copy Markdown
Member

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.

4 participants