Suppress Units warnings from Line Analysis plugin #1648
Conversation
rosteen
requested review from
duytnguyendtn,
javerbukh,
ojustino,
pllim and
kecnry
as code owners
Codecov ReportBase: 86.31% // Head: 86.76% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
+ Coverage 86.31% 86.76% +0.45%
==========================================
Files 95 95
Lines 9588 9652 +64
==========================================
+ Hits 8276 8375 +99
+ Misses 1312 1277 -35
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -276,6 +276,11 @@ def _on_identified_line_changed(self, msg): | |||
| # in which case we'll default to the identified line | |||
| self.selected_line = self.identified_line | |||
|
|
|||
| def _unit_ignore_warnings(self, unit): | |||
| with warnings.catch_warnings(): | |||
| warnings.simplefilter('ignore') | |||
There was a problem hiding this comment.
Do we want to be more specific here? This blanket ignore may hide real bugs.
There was a problem hiding this comment.
Maybe we do not even need this.
| @@ -446,7 +452,7 @@ def _uncertainty(result): | |||
| raw_result = analysis.line_flux(wave_spec) | |||
| # When flux is equivalent to Jy, lineflux result should be shown in W/m2 | |||
| if flux_unit.is_equivalent(u.Unit('W/m2/m'/u.sr)): | |||
| final_unit = u.Unit('W/m2/sr') | |||
| final_unit = self._unit_ignore_warnings('W/m2/sr') | |||
There was a problem hiding this comment.
We have full control of how the unit string is written here. Why not just change the string so it stops emitting warning?
| final_unit = self._unit_ignore_warnings('W/m2/sr') | |
| final_unit = u.Unit('W / (m2 sr)') |
There was a problem hiding this comment.
Huh, the original ticket said that warnings were being thrown from our use of "W / m2 / m", and I totally didn't even consider that the warning I was actually seeing was for "W / m2 / sr" which, as you said, can be rewritten. Let me think about this a little more, your point that we don't even need the filtering might be correct.
There was a problem hiding this comment.
W / m2 / sr may still be the preferred format for any user-facing representation of the unit... but if it's all internal, then that would definitely be the "easier" fix.
There was a problem hiding this comment.
p.s. We should not encourage users to write unit in a format that emits astropy warnings. 😉
There was a problem hiding this comment.
I was going to make the same point Kyle, this is really about how it's displayed to the user, and it may be that the desired format is the one with more slashes that naturally parses to "Watts per square meter per steradian". @PatrickOgle can you chime in on this?
There was a problem hiding this comment.
WARNING: UnitsWarning: 'W/m2/sr' contains multiple slashes, which is discouraged by the FITS standard 🤷
| @@ -425,7 +430,8 @@ def _uncertainty(result): | |||
| raw_result = analysis.line_flux(freq_spec) | |||
| # When flux is equivalent to Jy, lineflux result should be shown in W/m2 | |||
| if flux_unit.is_equivalent(u.Jy/u.sr): | |||
| final_unit = u.Unit('W/m2/sr') | |||
|
|
|||
| final_unit = self._unit_ignore_warnings('W/m2/sr') | |||
There was a problem hiding this comment.
| final_unit = self._unit_ignore_warnings('W/m2/sr') | |
| final_unit = u.Unit('W / (m2 sr)') |
| @@ -276,6 +276,11 @@ def _on_identified_line_changed(self, msg): | |||
| # in which case we'll default to the identified line | |||
| self.selected_line = self.identified_line | |||
|
|
|||
| def _unit_ignore_warnings(self, unit): | |||
| with warnings.catch_warnings(): | |||
| warnings.simplefilter('ignore') | |||
There was a problem hiding this comment.
Maybe we do not even need this.
|
@pllim I decided to remove the function and reformat things in a way that isn't "discouraged" by astropy, as you suggested. |
There was a problem hiding this comment.
Might want to update the change log to better reflect the current solution. But I agree this is cleaner since it's only internal code and not exposed to the user. Other than the change log, I just have one small question, but then hopefully we can get this in!
| if flux_unit.is_equivalent(u.Unit('W/m2/m'/u.sr)): | ||
| final_unit = u.Unit('W/m2/sr') | ||
| if flux_unit.is_equivalent(u.Unit('W/(m2 m)'/u.sr)): | ||
| final_unit = u.Unit('W/(m2 sr)') |
There was a problem hiding this comment.
does final_unit affect what is exposed to the user or is that logic handled elsewhere (do we need to override to show W/m2/sr in the UI)?
There was a problem hiding this comment.
This is how it will show up in the UI, but I didn't get any objections from POs. I figure get rid of the warnings this way for now, and if down the line people really hate that format I can bring back _unit_ignore_warnings.
Suppresses the
W/m2/mwarning from astropy.units when using the line analysis plugin.