Allow multiple cases when calculating power spectrum - #2376
Allow multiple cases when calculating power spectrum#2376Carol Halliwell (cehalliwell) wants to merge 15 commits into
Conversation
Total coverage: 93% (HTML report)Name Stmts Miss Branch BrPart Cover --------------------------------------------------------------------------------------------------- src/CSET/__init__.py 105 0 14 0 100% src/CSET/_common.py 156 0 54 0 100% src/CSET/cset_workflow/app/fetch_fcst/bin/fetch_data.py 117 28 26 0 78% src/CSET/cset_workflow/app/fetch_nimrod/bin/fetch_nimrod.py 81 8 28 11 83% src/CSET/cset_workflow/app/finish_website/bin/finish_website.py 79 2 8 2 95% src/CSET/cset_workflow/app/parbake_recipes/bin/parbake.py 29 0 8 0 100% src/CSET/cset_workflow/app/send_email/bin/send_email.py 25 0 4 0 100% src/CSET/cset_workflow/lib/python/jinja_utils.py 17 0 6 0 100% src/CSET/extract_workflow.py 103 1 26 1 98% src/CSET/graph.py 44 0 14 0 100% src/CSET/operators/__init__.py 89 0 26 0 100% src/CSET/operators/_atmospheric_constants.py 9 0 0 0 100% src/CSET/operators/_colormaps.py 249 3 72 4 98% src/CSET/operators/_stash_to_lfric.py 3 0 0 0 100% src/CSET/operators/_utils.py 190 8 72 6 95% src/CSET/operators/ageofair.py 142 7 64 5 94% src/CSET/operators/aggregate.py 77 1 22 1 98% src/CSET/operators/aviation.py 61 0 18 0 100% src/CSET/operators/collapse.py 155 8 72 3 93% src/CSET/operators/constraints.py 113 7 50 3 93% src/CSET/operators/convection.py 38 4 10 2 88% src/CSET/operators/ensembles.py 27 0 14 0 100% src/CSET/operators/feature.py 44 0 10 0 100% src/CSET/operators/filters.py 67 2 30 0 98% src/CSET/operators/fluxes.py 41 0 10 0 100% src/CSET/operators/humidity.py 135 0 52 0 100% src/CSET/operators/imageprocessing.py 57 0 16 0 100% src/CSET/operators/mesoscale.py 18 0 2 0 100% src/CSET/operators/misc.py 172 1 72 3 98% src/CSET/operators/plot.py 1121 156 416 72 83% src/CSET/operators/power_spectrum.py 155 15 62 9 87% src/CSET/operators/precipitation.py 204 2 92 2 99% src/CSET/operators/pressure.py 41 0 12 0 100% src/CSET/operators/read.py 430 21 182 16 94% src/CSET/operators/regrid.py 147 1 70 3 98% src/CSET/operators/scoreswrappers.py 208 16 58 6 91% src/CSET/operators/temperature.py 121 0 32 0 100% src/CSET/operators/transect.py 63 0 24 0 100% src/CSET/operators/wind.py 46 3 10 2 91% src/CSET/operators/write.py 15 0 6 0 100% src/CSET/recipes/__init__.py 104 0 28 0 100% src/CSET/sample_data/__init__.py 0 0 0 0 100% --------------------------------------------------------------------------------------------------- TOTAL 5098 294 1792 151 93% |
|
The function calculate_power_spectrum has been expanded so that it also loops over the forecast_reference_time allowing for multiple cases in addition to realizations. A test has been written that checks that the power spectrum is calculated for a cube with multiple forecast_reference_times and realizations. Microsoft Copilot was used for code suggestions. |
|
Example for multiple cases 
|
James Warner (jwarner8)
left a comment
James Warner (jwarner8)
left a comment
There was a problem hiding this comment.
Looks good, small suggestion where it might fail.
Other thing is that it would be good to have some brief comments at different stages of code to describe what it does (I think the new code has also dropped some of the old comments too, e.g. old L90/91, L96
Co-authored-by: James Warner <62252918+jwarner8@users.noreply.github.com>
|
Code has now been changed following discovery that concatenation of cubes needs to be done in stages within calculate_power_spectrum when both realization and forecast_reference_time coordinates exist. Code now checks whether data has realization and/or forecast_reference_time coordinates before build a listing containing: Pytest test_plot_line_series_extra_dimension_failure had be updated so that a dummy coordinate is added to a test cube, rather than forecast_reference_time to ensure cube has greater than 2 dimensions (forecast_reference_time is ignored when counting dimensions for the purpose of the check). Microsoft Copilot was used for code suggestions and checks. Code is ready for review again. |
James Warner (jwarner8)
left a comment
James Warner (jwarner8)
left a comment
There was a problem hiding this comment.
Looking good, few minor edits and a few more tests (sorry!) to increase code coverage and check the raises work OK.
| ) from err | ||
| # Count dimensions excluding realization | ||
| # Count dimensions excluding realization and forecast_reference_time | ||
| ndim = model_cube.ndim |
There was a problem hiding this comment.
If multiple forecasts, wouldn't forecast_reference_time be promoted from a scalar to a dimension, and thus picked up in ndim?
There was a problem hiding this comment.
ah, I see code below then subtracts ndim if forecast reference_time. Might be worth a comment on this line saying it will be adjusted if these exist later in code
|
|
||
| time_dims = frt_cube.coord_dims("time") | ||
|
|
||
| if len(time_dims) != 1: |
There was a problem hiding this comment.
be good to have a test that covers this, mocking up a simple cube
|
|
||
| frt_dims = frt_cube.coord_dims("forecast_reference_time") | ||
|
|
||
| if len(frt_dims) != 1: |
There was a problem hiding this comment.
test for this?
| combined_cube = cubes_for_frt_concat.concatenate_cube() | ||
|
|
||
| else: | ||
| # If all FRT cubes have the same time coordinate, |
There was a problem hiding this comment.
Code coverage suggests this doesn't get touched by tests, good to cover this along with making sure correct raise if error
2 participants
The power spectrum code currently fails when selecting multiple cases. This PR aims to fix this by looping over forecast_reference_time in function calculate_power_spectrum.
Linked to Issue #215
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.