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

Force flux_unit to be unit instance #403

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jan 6, 2025

Force flux_unit to be unit instance instead of a str to be compatible with astropy v7.1

xref astropy/astropy#17586

instead of a str to be compatible with astropy v7.1
@pllim pllim added this to the 1.6.0 milestone Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

Thank you for your contribution! 🌌 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 STScI coding guidelines?
  • Are tests added/updated as required? If so, do they follow the STScI testing guidelines?
  • Are docs added/updated as required?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions.
  • Did the CI pass? If no, are the failures related?
  • Is a change log needed?

Copy link

github-actions bot commented Jan 6, 2025

👋 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?

@mhvk
Copy link

mhvk commented Jan 6, 2025

A bit less obviously also an improvement in the code than for jdaviz - while you are at it, does it make sense to also re-assign in the one other case where self._validate_flux_unit is called? (I just did a code search, so am not sure whether it is in fact needed)

@pllim
Copy link
Contributor Author

pllim commented Jan 6, 2025

You mean here?

self._validate_flux_unit(pval.unit)
outval = units.convert_flux(self._redshift_model(wave), pval,
self._internal_flux_unit).value

In this case, pval is already a Quantity, so we do not have any problem with string.

@pllim pllim marked this pull request as ready for review January 6, 2025 18:30
@mhvk
Copy link

mhvk commented Jan 6, 2025

Yes, sorry I didn't look better before commenting!

@pllim pllim merged commit 47a0396 into spacetelescope:master Jan 6, 2025
20 checks passed
@pllim pllim deleted the fix-ci branch January 6, 2025 18:46
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.19%. Comparing base (262a309) to head (2466fe3).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #403   +/-   ##
=======================================
  Coverage   97.19%   97.19%           
=======================================
  Files          17       17           
  Lines        2031     2031           
=======================================
  Hits         1974     1974           
  Misses         57       57           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

2 participants