-
Notifications
You must be signed in to change notification settings - Fork 7
Regional power spectra using Discrete Cosine Transform (DCT) #1765
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
base: main
Are you sure you want to change the base?
Conversation
|
Power spectra calculated over regional (limited area) domain using Discrete Cosine Transform (DCT). This code is based on Denis et al 2002. The code calculates power spectrum on a single (surface, pressure or model) level for each specified time/level but does not yet allow aggregation or consider ensembles. Mean spectra over a specified time period for each level and/or mean spectra over a range of levels at a specified time may be future options. M365 Copilot was used to optimise the code and give code suggestions. |
|
Power Spectrum code changed so that the power spectrum is now calculated in _plot_and_save_power_spectrum_series rather than a separate operator. This is so results from multiple models can be plotted on the same plot. The unit tests for the power spectrum code have now been updated to reflect these changes. There are currently only 2 recipes: which allow plotting of power spectrum on model, pressure and surface levels. There is no method of aggregation at the moment. This might involve taking mean of spectra over times or levels but needs further investigation. |
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.
This PR is looking good. I've got a few comments here, and some more specific ones in the code.
It is worth considering tests for the following: 3D data, a cubelist, no coordinate named time present (if allowing it, see comments in code), a multi-model run, a test to cover the other options of L2649, L2692, and L2702of plot.py if those if statements are needed.
It would be nice to include somewhere (either in the function description, or the recipe description) a quick guide to interpretation of power spectra.
You also need to update the conda lock files too: Actions -> Update conda lock files -> Run workflow, select your branch, and then merge in the PR that the bot creates into your PR.
Finally, it might be worth getting someone else to do a technical review of this PR as well, or discuss with @jfrost-mo whether the power spectra code should be separate from the plotting code. My only thought behind this is that for aggregation purposes it might be easier to have the calculation of the power spectra separate.
It's really great to see progress on this, and the output looks fantastic.
| variables={ | ||
| "VARNAME": field, | ||
| "MODEL_NAME": [model["name"] for model in models], | ||
| "SEQUENCE": "time" |
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.
Applies to all three loaders here. Should the SEQUENCE be set to conf.SPECTRUM_SURFACE_FIELD_SEQUENCE? Otherwise won't this setting overwrite what is in there recipe and always do a sequence rather than the FALSE option? Or is this a temporary thing related to lack of aggregation?
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.
This is a copy of the loader for histograms, basically replacing "HISTOGRAM" with "SPECTRUM" and removing anything relating to aggregation at the moment. Maybe I misunderstand what the code but I thought conf.SPECTRUM_SURFACE_FIELD_SEQUENCE was a logical and "SEQUENCE" was specifying the coordinate, which would always be time?
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.
Do we actually need that logical then? I don't see it being used anywhere so does the logical come in with aggregation (i.e. aggregation across a model run) and so should be included later?
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'm not sure. Maybe a question for whoever wrote the loader for the histograms? I don't see it in the aggregation section of the histogram loader.
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 suppose one thing to try is running it with and without that switch to False and see what happens. It may be more of a question for who put that functionality into CSET rather than the loader, as the loader was based on the original includes file.
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.
It ran with that set to False when using cset bake. (?!)
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.
Ir ran with the switch set to False. ?!?
| "LEVELTYPE": "pressure", | ||
| "LEVEL": plevel, | ||
| "MODEL_NAME": [model["name"] for model in models], | ||
| "SEQUENCE": "time" |
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.
See question above.
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.
See comment above
| "LEVELTYPE": "model_level_number", | ||
| "LEVEL": mlevel, | ||
| "MODEL_NAME": [model["name"] for model in models], | ||
| "SEQUENCE": "time" |
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.
See question above.
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.
See comment above
| # If cube is a CubeList, extract the first cube | ||
| if isinstance(cube, iris.cube.CubeList): | ||
| cube = cube[0] |
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.
| # If cube is a CubeList, extract the first cube | |
| if isinstance(cube, iris.cube.CubeList): | |
| cube = cube[0] |
I don't think this is needed any more? cubes will be a cube/cubelist and iter_maybe will loop over each cube within the cubelist, so will produce this result. Or is it aiming at something else?
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 think you are right - it was necessary in an earlier version of the code but I moved it into the for loop. I have removed that section now.
| elif cube.ndim == 3: | ||
| cube_3d = cube.data | ||
| else: | ||
| raise ValueError("Cube dimensions unsuitable for power spectra code") |
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.
| raise ValueError("Cube dimensions unsuitable for power spectra code") | |
| raise ValueError(f"Cube is {cube.ndim} dimensional. Cube should be 2 or 3 dimensional.") |
This error message is a little more useful as it tells the user how the cube is unsuitable and what needs to be done to fix the error.
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 agree. I have added the extra message. Thanks.
| if isinstance(cube_slice, iris.cube.CubeList): | ||
| single_cube = cube_slice[0] |
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.
A little confused around this section? The cubes may not always be read in the same order so you could end up changing cubes by selecting the first cube_slice. In what instances will the slice be a cubelist?
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.
Again, this is copied from the histogram code. When running the code, this is loop is accessed (I checked with a print statement)
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.
The code does not work without this and, checking with a print statement, this if statement is accessed, implying that it can be a cubelist.
| return cubes | ||
|
|
||
|
|
||
| def DCT_ps(y_3d): |
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.
| def DCT_ps(y_3d): | |
| def _DCT_ps(y_3d): |
If we are not using this function within other recipes (i.e. only using it as part of a plotting function) we could define this as a non-public function.
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.
Yes, that makes sense. Changing the function to non-public.
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.
You are right. Function changed to non-public. Thanks.
| # Regional domains: | ||
| # Calculate power spectra over limited are domain using Discrete Cosine Transform (DCT) | ||
| # as described in Denis et al 2002 [Denis_etal_2002]_. | ||
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.
Useful to include a description of y_3d.
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.
Good point. I've added description of input (y_3d) and output (ps_array).
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 have expanded the description in the _DCT_ps function.
| return ps_array | ||
|
|
||
|
|
||
| def create_alpha_matrix(Ny, Nx): |
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.
| def create_alpha_matrix(Ny, Nx): | |
| def _create_alpha_matrix(Ny, Nx): |
Again I don't think this needs to be a public function as not used elsewhere.
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.
Changed to non-public
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 have made this function non-public.
| Each pair is associated with a single-scale parameter. alpha_matrix is normalisation of | ||
| 2D wavenumber axes, transforming the spectral domain into an elliptic coordinate system. | ||
| """ |
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.
Useful to have a description of the Nx and Ny parameters.
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.
Description expanded in _create_alpha_matrix

Follows on from #215
Calculate power spectra of 2D field in regional model using Discrete Fourier Transform (DCT)
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.