Moment units redux #2588
Moment units redux #2588
Conversation
rosteen
requested review from
javerbukh,
pllim,
kecnry,
bmorris3,
haticekaratay and
cshanahan1
as code owners
|
Looks like tests need updating for the new reduced units. The |
|
I'm also still debugging something I found after opening the PR, I'll fix tests once I'm done that. |
|
Here's a quick demo of what this looks like now: Screen.Recording.2023-12-05.at.4.12.10.PM.mov |
| # Filter what we want based on n_moment | ||
| if self.n_moment == 0: | ||
| self.output_radio_items = [self.output_unit_items[0],] | ||
| elif self.n_moment == 1: | ||
| self.output_radio_items = self.output_unit_items[1:3] | ||
| else: | ||
| self.output_radio_items = self.output_unit_items[2:] |
There was a problem hiding this comment.
how does this look to the user using the API?
There was a problem hiding this comment.
The user still sees the full options list in the API.
There was a problem hiding this comment.
If anything isn't valid, we should raise errors in calculate_moment then?
There was a problem hiding this comment.
Added a check for that.
There was a problem hiding this comment.
This seems quite intuitive from the UI when changing moment (it remembers the user selection when it still exists, but defaults to a new value when it doesn't).
Screen.Recording.2023-12-07.at.12.46.54.PM.mov
From the API, there are still a few weird cases, but those handled by the error checking in calculate_moment, which should be sufficient.
| # Initialize extra key in items dictionary | ||
| for item in self.output_unit_items: | ||
| item["unit_str"] = "" |
There was a problem hiding this comment.
this might not be necessary since vue should handle the key missing
There was a problem hiding this comment.
Looks like you're right, I'll remove this.
| @click:action="calculate_moment" | ||
| ></plugin-add-results> | ||
|
|
||
| <v-row v-if="n_moment > 0 && output_unit_selected === 'Velocity' && reference_wavelength === 0"> | ||
| <v-row v-if="n_moment > 0 && output_unit_selected !== 'Spectral Unit' && reference_wavelength === 0"> | ||
| <span class="v-messages v-messages__message text--secondary" style="color: red !important"> | ||
| Cannot calculate moment: Must set reference wavelength for output in velocity units. |
There was a problem hiding this comment.
I can get this same error message by setting n_moment to 1 and output_unit to Flux... granted that is only possible by the UI, so I'm not sure its worth an extra case here to show the correct error message or not.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2588 +/- ##
==========================================
- Coverage 91.55% 91.54% -0.01%
==========================================
Files 161 161
Lines 19842 19932 +90
==========================================
+ Hits 18166 18247 +81
- Misses 1676 1685 +9 ☔ View full report in Codecov by Sentry. |
I added a separate case in the vue template for this, good suggestion to make this more accurate. |
Co-authored-by: Brett M. Morris <morrisbrettm@gmail.com>
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
rosteen
force-pushed
the
moment-units-redux
branch
from
2b142ef to
b38ffec
Compare
Updates the recent Moment Maps unit work to better align with PO specs by displaying the actual physical units (including flux units for moment 0), and giving the option to reduce all higher moments to velocity rather than velocity^N.