Add option to calculate higher-order moments in velocity #2584
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2584 +/- ##
==========================================
+ Coverage 91.51% 91.53% +0.02%
==========================================
Files 161 161
Lines 19542 19592 +50
==========================================
+ Hits 17883 17934 +51
+ Misses 1659 1658 -1 ☔ View full report in Codecov by Sentry. |
rosteen
requested review from
javerbukh,
pllim,
kecnry,
bmorris3,
haticekaratay and
cshanahan1
as code owners
|
Tests added, docs updated, good to go (well, ready for review at least). |
| @@ -62,6 +69,11 @@ def __init__(self, *args, **kwargs): | |||
|
|
|||
| self.moment = None | |||
|
|
|||
| self.output_unit = SelectPluginComponent(self, | |||
There was a problem hiding this comment.
do we want this - and probably reference_wavelength as well - added to the user API (and docstring, and update tests)?
There was a problem hiding this comment.
Good call, I can do that after tagup.
Is this still the case? |
No, the code works for arbitrary |
Co-authored-by: Ricky O'Steen <[email protected]>
Co-authored-by: Kyle Conroy <[email protected]>
Don't need self here
rosteen
force-pushed
the
moments-like-tears-in-velocity
branch
from
eaf0233
to
ebc7a2f
Compare
| @@ -35,6 +35,37 @@ | |||
| ></v-text-field> | |||
| </v-row> | |||
|
|
|||
| <div v-if="n_moment > 0 && dataset_spectral_unit !== ''"> | |||
There was a problem hiding this comment.
I'm a little torn by the fact that this isn't checked for in the API at all, but I think that's probably ok since the idea is that the plugin APIs just let you modify inputs. Something like #2347 would address this concern in general. The other option would be to have changing n_moment to 0, automatically revert output_unit = 'wavelength' under-the-hood, but then that could be confusing from the UI-perspective to see it reset when setting a positive moment again.
Definitely doesn't need to hold up approval/merge, just wanted to leave a papertrail of thoughts.
| @@ -51,6 +58,10 @@ class MomentMap(PluginTemplateMixin, DatasetSelectMixin, SpectralSubsetSelectMix | |||
| filename = Unicode().tag(sync=True) | |||
| moment_available = Bool(False).tag(sync=True) | |||
| overwrite_warn = Bool(False).tag(sync=True) | |||
| output_unit_items = List().tag(sync=True) | |||
| output_unit_selected = Unicode().tag(sync=True) | |||
| reference_wavelength = FloatHandleEmpty().tag(sync=True) | |||
There was a problem hiding this comment.
this defaults to zero, but since its the original value, does not result in a validation error on the input, and ultimately results in a traceback (to avoid infinites in the velocities) 🤔
We could have this update to the central value of the selected data/subset.. but then that could override a user-provided value unless we had some logic like we do for labels. I think our best bet for now is to validate the inputs at the "calculate" button-level (probably can be done on the vue-side since it just needs reference_wavelength, output_unit_selected and n_moment) and disable the button with explanation text (we usually use <span class="v-messages v-messages__message text--secondary" style="color: red !important"> for situations like this elsewhere).
Thanks! |
This currently works for moment 1, but I need to add handling for higher orders since those will end up as
m2/s2(for order 2) and so on. Also still need to add tests.