-
Notifications
You must be signed in to change notification settings - Fork 5
Update cset-workflow GUI organisation with clearer labelling for "General setup", "Cycling and Models", diagnostic-type and evaluation-type specific option panels #1310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Testing: Sample outputs can be seen at https://wwwspice/~huw.lewis/CSET/CSET_WAfr_RAL3LFRic/ If wishing to explore rose GUI locally, try rose edit -C /home/users/huw.lewis/CSET/CSET/cset-workflow Sample screenshots (compare with originals in issue): Diagnostic options home panel (would allow expansion to include future aspects like specifying subarea selection, regridding etc): Separating out "surface fields", "pressure fields", "model level fields" panels: Capturing new panel for any diagnostics in "other" as implying more bespoke diagnostic methods, that propose should assume to have their own GUI panel rather than invite users to look through potentially long future list of disparate options connected to different methods etc: Documentation |
Recognise likely conflicts with #1303, but think proposed changes may make visibility of radio buttons for histograms (and other future additions to plotting options) easier to navigate and implement etc. |
My cset-workflow/run19 testing (with James W Africa data) failed on following tasks:
All due to "Histogram can only plot a single cube" errors. |
Sense check test GUI requests vs output: PLOT_SPATIAL_SURFACE_MODEL_FIELD_CASE_AGGREGATION: by lead time + hour of day no aggregation produced - will need to check logic SPATIAL_SURFACE_DIFFERENCE_CASE_AGGREGATION: by lead time + hour of day no aggregation produced - will need to check logic DOMAIN_MEAN_SURFACE_TIME_SERIES_CASE_AGGREGATION: by lead time + hour of day no aggregation produced - will need to check logic DOMAIN_SURFACE_HISTOGRAM_SERIES_FIELD_CASE_AGGREGATION: by lead time + all cases some aggregation output - looks like it's the "all cases" option that's worked here Pressure level fields and profiles similarly lacking case aggregation outputs just now. I suspect something not following through from my proposed includes/case_aggregation_options.cylc (noting I wanted to keep that away from the main flow.cylc file if I could, or may be preferable just to apply some of this logic in relevant includes/ file if looks more complex than needed?) UPDATE: Have now pushed translation of rose-suite.conf T/F statements into flow.cylc and appear to be picked up correctly (see following change update). |
rose-suite.conf inputs
Sense check test GUI requests vs output 2: Sample outputs: https://wwwspice/~huw.lewis/CSET/CSET_WAfr_RAL3LFRic/ PLOT_SPATIAL_SURFACE_MODEL_FIELD_CASE_AGGREGATION: by lead time + hour of day --> aggregation produced by lead time and hour of day as requested PRESSURE_LEVEL SPATIAL --> produced by lead time + hour of day as requested SPATIAL_SURFACE_DIFFERENCE_CASE_AGGREGATION: by lead time + hour of day --> produced both as requested DOMAIN_MEAN_SURFACE_TIME_SERIES_CASE_AGGREGATION: by lead time + hour of day --> both produced as requested, though appear under 'Time Series' tab in CSET webpage rather than 'Surface Time Series' DOMAIN_SURFACE_HISTOGRAM_SERIES_FIELD_CASE_AGGREGATION: by lead time + all cases some aggregation output --> both generated as requested All appears to have functioned correctly |
Note opt rose-suite.conf files will need updating to reflect any changes to workflow options etc, but want to ensure we bottom out approach first before making those changes. |
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 haven't gone into too much detail on the minutia yet, but I wanted to write down my thoughts on the overall design.
I really like the improvements to the output. The ordering is significantly nicer and the additional categorisation of diagnostics definitely improves their discoverability. Having multiple checkboxes on a single line is a novel way of selecting multiple different kinds of aggregation without making the list too long.
I can't say I'm much of a fan of the translation table in the flow.cylc. I worry we have too much Jinja2 logic already, without adding configuration parsing too. On the other hand this is very simple logic, so probably doesn't hurt testability too much, and does provide a very real benefit. I also can't think of another way to do this that is any nicer.
I'm not sure if we need to keep the toggle for showing the case aggregation options now we have more granular categories. It was implemented as a stopgap measure when the quicklooks section became overwhelming, but this redesign has solved that problem more fundamentally.
This may be slight scope creep, but it would be good to provide help text for the diagnostics, though I do wonder if this would be better in the web documentation instead (or in addition). Also nicer titles could be set.
Finally aligning the cylc task name with the recipe name is a very sensible change, but I wonder if we could go further. If we aligned the name of the include file with the recipe it would pave the way to being able to auto-generate them. We could even use the "Category" information in the recipe to construct the configuration. This is probably something for a subsequent pull request though.
Co-authored-by: James Frost <[email protected]>
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.
Happy to approve these changes. The only change that needs to happen before merging is to edit the pull request title to be a concise description of the changes in this PR, so it can be pulled through to use in the changelog.
Reviewed and refined rose GUI layout.
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.