Skip to content

Don't log when passed unit matches FITS BUNIT#19389

Open
m4cd4r4 wants to merge 6 commits intoastropy:mainfrom
m4cd4r4:fix/ccddata-redundant-log
Open

Don't log when passed unit matches FITS BUNIT#19389
m4cd4r4 wants to merge 6 commits intoastropy:mainfrom
m4cd4r4:fix/ccddata-redundant-log

Conversation

@m4cd4r4
Copy link
Copy Markdown

@m4cd4r4 m4cd4r4 commented Mar 10, 2026

Description

Description

Don't log when passed unit matches FITS BUNIT

CCDData.read logs an info message whenever a unit argument is passed alongside a FITS file that has a BUNIT header, even when they match. This is noisy when users explicitly pass the unit to confirm what's already in the file.

The log now only fires when the two units actually differ.

Fixes #13539

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions
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 instructions for rebase and squash.
  • 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?
  • 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.

@pllim pllim added this to the v8.0.0 milestone Mar 10, 2026
@pllim
Copy link
Copy Markdown
Member

pllim commented Mar 10, 2026

Probably need regression test.

@m4cd4r4
Copy link
Copy Markdown
Author

m4cd4r4 commented Mar 20, 2026

Hi @pllim - the regression test was added in commit 9a80dda (March 12). It covers the case where the passed unit matches FITS BUNIT and verifies no log message is emitted. All CI is green. Happy to make any adjustments if needed.

Copy link
Copy Markdown
Member

@mwcraig mwcraig left a comment

Choose a reason for hiding this comment

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

Thanks for opening this, @m4cd4r4. Just some minor changes...

fits_unit_parsed = u.Unit(fits_unit_string)
except ValueError:
fits_unit_parsed = None
if fits_unit_parsed is None or fits_unit_parsed != u.Unit(unit):
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 would prefer a comparison of the string units as in #18678 rather than trying to parse the unit. There are some "valid" fit unit strings that do not immediately translate into astropy units.

@m4cd4r4
Copy link
Copy Markdown
Author

m4cd4r4 commented Mar 24, 2026

Done - switched to str(unit) \!= str(fits_unit_string) to match the approach in #18678. Removed the try/except block and unit parsing entirely.

@mwcraig
Copy link
Copy Markdown
Member

mwcraig commented Mar 25, 2026

Wonderful, thanks @m4cd4r4! Two more quick things:

  • Can you please rebase? Looks like this is out of date with main.
  • Could you please add @Warnocke as a co-author on the last commit? That won't change who gets credit for this PR but will acknowledge their earlier work.

@m4cd4r4 m4cd4r4 force-pushed the fix/ccddata-redundant-log branch from 34c476d to f72e2f8 Compare March 25, 2026 22:47
@m4cd4r4
Copy link
Copy Markdown
Author

m4cd4r4 commented Mar 25, 2026

Done - rebased onto main and added @Warnocke as co-author on the last commit.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid logger info in CCDData.read when unit specifies matches that in FITS file

3 participants