-
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
Future-proof physical_type usage for astropy 4.3 #550
Conversation
spectral_axis_unit_type = data.spectral_axis.unit.physical_type.title() | ||
try: | ||
spectral_axis_unit_type = data.spectral_axis.unit.physical_type.title() | ||
except AttributeError: |
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.
except
is expensive. Better to check for astropy
version on import and use if ... else ...
based on that check.
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.
I also wonder if list(data.spectral_axis.unit.physical_type)[0].title()
would be better, in case you are accessing a unit with multiple entries, but if that is not a possibility, then you don't have to.
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.
LGTM
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.
LGTM. Thanks!
p.s. The protection rule for "outdated branch" might be overkill. Made me press a button that created a merge commit. |
Agreed, I was wondering where that came from when I had to do it for one of your PRs. |
Fixes #549 .