-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Disable export for unsupported units #2784
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2784 +/- ##
==========================================
+ Coverage 88.72% 88.88% +0.15%
==========================================
Files 110 110
Lines 16353 16505 +152
==========================================
+ Hits 14509 14670 +161
+ Misses 1844 1835 -9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small comment, otherwise looks good to me once tests pass!
e49ca6d
to
eebf2e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved, pending small comments
if self.dataset.selected_obj.meta.get("Plugin", None) is None: | ||
# NOTE: should not be a valid choice due to dataset filters, but we'll include | ||
# another check here. | ||
self.data_invalid_msg = "Data export is only available for plugin generated data." | ||
elif not isinstance(self.dataset.selected_obj, (Spectrum1D, CCDData)): | ||
self.data_invalid_msg = "Export is not implemented for this type of data" | ||
elif data_unit == u.Unit('DN/s'): | ||
elif (data_unit := self.dataset.selected_obj.unit) == u.Unit('DN/s'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WALRUS!!! 🎉 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The Walrus! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is a walrus?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhh := := :=
Description
This pull request disables writing data products to FITS files due to unsupported DN/s units in Astroquery's internal.
Fixes #
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.