Skip to content

Refactor to_string in the unit formats - #14416

Merged
pllim merged 6 commits into
astropy:mainfrom
mhvk:units-format-refactor
Feb 22, 2023
Merged

Refactor to_string in the unit formats#14416
pllim merged 6 commits into
astropy:mainfrom
mhvk:units-format-refactor

Conversation

@mhvk

@mhvk mhvk commented Feb 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This pull request refactors the output in the units.format classes, making to_string() simpler.

Right now there is no behaviour change except that the unused "unscaled" class is deprecated (removing a test for it that I had just added).

EDIT: this PR is too complex, and I decided to break it up in parts (leaving this as draft to give a sense where it is going, and perhaps still use it for the wrap-up):

EDIT: possible follow-up:

@mhvk mhvk added this to the v5.3 milestone Feb 19, 2023
@github-actions

github-actions Bot commented Feb 19, 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 Feb 19, 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?

This was referenced Feb 19, 2023
Merged
Merged
@mhvk
mhvk force-pushed the units-format-refactor branch 2 times, most recently from 1a7936f to de24d8c Compare February 21, 2023 17:19
@mhvk
mhvk marked this pull request as ready for review February 21, 2023 17:19
@mhvk
mhvk requested review from nstarman and olebole February 21, 2023 17:20
@mhvk

mhvk commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

For reviewers: it may help to look commit by commit - they are more or less logically organized...

nstarman
nstarman reviewed Feb 21, 2023
View reviewed changes

@nstarman nstarman 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 think when this PR was rebased it lost the explanatory comments, e.g. in def to_string(cls, unit, inline=False):

Comment thread astropy/units/format/latex.py Resolved
@mhvk

mhvk commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

Oops! Will get those back!

@mhvk
mhvk force-pushed the units-format-refactor branch from de24d8c to 03146c3 Compare February 21, 2023 21:32
@mhvk

mhvk commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

OK, those comments are back. While going through where exactly they were dropped, I noticed that I had also dropped useful comments in latex's description of why we typeset units like degrees differently when they are raised to a power. So, reinstated that too.

nstarman
nstarman reviewed Feb 22, 2023
View reviewed changes

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

This PR looks good. My comments are minor and mostly focused on speed.

Comment thread astropy/units/format/base.py Resolved
Comment thread astropy/units/format/base.py Resolved
Comment thread astropy/units/format/base.py Resolved
Comment thread astropy/units/format/base.py
return name

@classmethod
def _format_unit_list(cls, units):

@nstarman nstarman Feb 22, 2023

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 think a tuple will always work here. Perhaps this should be changed since tuples are faster and more memory efficient.

@mhvk mhvk Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Did you think we should change the name? (I think better to leave it the same)

@nstarman nstarman Feb 22, 2023

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.

It's private, so we're free to change it. And it's more explicit and will help when we add typing. I would be inclined to change the name.

@mhvk mhvk Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

utils.grouped_by_powers does return lists for nominator and denominator (constructed by appending, so logical). So, _format_unit_tuple is incorrect.

Actually, looking at the override ingeneric - that does a .sort, so it does assume a list. Frankly, generic with inline=True looks awful, but that is for the next PR... For now, let me just revert those tuples to lists (and add a test...)

@nstarman nstarman Feb 22, 2023

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.

SGTM. I suppose allowing lists, but using tuples wherever possible is good enough.

Comment thread astropy/units/format/base.py Resolved
@mhvk
mhvk force-pushed the units-format-refactor branch from 03146c3 to 9351845 Compare February 22, 2023 14:40
@mhvk

mhvk commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

CI failures seem unrelated (asdf in -dev)

@WilliamJamieson

WilliamJamieson commented Feb 22, 2023

Copy link
Copy Markdown
Contributor

CI failures seem unrelated (asdf in -dev)

The fix is in #14429.

olebole
olebole approved these changes Feb 22, 2023
View reviewed changes

@olebole olebole 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 no issues here, for me this all looks very nice and IMO make the unit formatting code much better to maintain. Thank you!

@pllim pllim mentioned this pull request Feb 22, 2023
Closed
@pllim

pllim commented Feb 22, 2023

Copy link
Copy Markdown
Member

There is still an unchecked checkbox in the original post. So, is this PR ready now or still waiting for something else?

@mhvk
mhvk force-pushed the units-format-refactor branch from 9351845 to ddcaac6 Compare February 22, 2023 17:23
@mhvk

mhvk commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

@pllim - I think all is ready now. Next PRs might actually change things...

pllim
pllim approved these changes Feb 22, 2023
View reviewed changes

@pllim pllim 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 don't grok the internals but changes to the expected test results LGTM, so I am approving by proxy. I'll let nstarman do a final review as well.

Thanks, everyone!

@mhvk

mhvk commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

@pllim - CI seemed stuck at a codecov upload; I cancelled that part but don't seem able to just restart that job. Do we have to restart the whole lot?

@nstarman - are you OK with merging?

@pllim

pllim commented Feb 22, 2023

Copy link
Copy Markdown
Member

CI seemed stuck at a codecov upload

Are you sure? I don't see any failure. The red "x" is from -0.03% decrease.

@mhvk

mhvk commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

The one stuck is "CI / tests / Python 3.9 with all optional dependencies (MacOS X) (pull_request) " https://github.com/astropy/astropy/actions/runs/4245294951/jobs/7380620766

@pllim

pllim commented Feb 22, 2023

Copy link
Copy Markdown
Member

I reran the "failed" ones. That job does not do coverage. Not sure why it took so long though.

nstarman
nstarman approved these changes Feb 22, 2023
View reviewed changes

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

Thanks @mhvk! LGTM

@pllim
pllim merged commit 92a3af7 into astropy:main Feb 22, 2023
@mhvk
mhvk deleted the units-format-refactor branch February 22, 2023 20:26
@mhvk

mhvk commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Weird, as you can see from the log, it did try to upload to codecov, which is what got stuck. Anyway, happy it worked on a second go!

@pllim

pllim commented Feb 22, 2023

Copy link
Copy Markdown
Member

I think this is a bug. It is trying to upload even when no coverage was computed:

['error'] There was an error running the uploader: No coverage files located, please try use `-f`, or change the project root with `-R`

@pllim

pllim commented Feb 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

We use that OpenAstronomy template now in Actions, so I think I'll have to report this upstream. See OpenAstronomy/github-actions-workflows#109

@mhvk mhvk mentioned this pull request Feb 22, 2023
Merged
@mhvk

mhvk commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olebole olebole olebole approved these changes
@pllim pllim pllim approved these changes
@nstarman nstarman nstarman approved these changes

Assignees

No one assigned

Labels

no-changelog-entry-needed Refactoring units

Projects

None yet

Milestone

v5.3

Development

Successfully merging this pull request may close these issues.

5 participants

@mhvk @WilliamJamieson @pllim @olebole @nstarman