2164 add dfss operator and plots - #2258
James Ross (james-a-f-ross) wants to merge 58 commits into
Conversation
|
James Frost (@jfrost-mo) David Flack (@daflack) Hi both, I am not finished totally with the unit tests and there are a couple things to polish up in here. I feel like it might be good to just get another pair of eyes on a few things especially with the plotting stuff as I've needed to add a couple of my own plotting operators. |
| values="centile", "threshold" | ||
| trigger=template variables=CENTILE: "centile"; | ||
| template variables=THRESHOLD: "threshold"; | ||
| compulsory=true |
There was a problem hiding this comment.
Might be nice to have both this one and the centile one as a list so can put multiple options in?
Total coverage: 92% (HTML report)Name Stmts Miss Branch BrPart Cover --------------------------------------------------------------------------------------------------- src/CSET/__init__.py 104 0 14 0 100% src/CSET/_common.py 149 0 52 0 100% src/CSET/cset_workflow/app/fetch_fcst/bin/fetch_data.py 116 28 26 0 77% src/CSET/cset_workflow/app/fetch_nimrod/bin/fetch_nimrod.py 80 8 28 11 82% 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 3 26 3 95% src/CSET/graph.py 43 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 243 1 64 2 99% src/CSET/operators/_stash_to_lfric.py 3 0 0 0 100% src/CSET/operators/_utils.py 193 8 76 6 95% src/CSET/operators/ageofair.py 141 7 64 5 94% src/CSET/operators/aggregate.py 76 1 22 1 98% src/CSET/operators/aviation.py 60 0 18 0 100% src/CSET/operators/collapse.py 154 8 72 3 93% src/CSET/operators/constraints.py 113 7 50 3 93% src/CSET/operators/convection.py 37 4 10 2 87% src/CSET/operators/dfss.py 171 20 38 7 87% src/CSET/operators/ensembles.py 27 0 14 0 100% src/CSET/operators/feature.py 41 0 10 0 100% src/CSET/operators/filters.py 66 2 30 0 98% src/CSET/operators/fluxes.py 41 0 10 0 100% src/CSET/operators/humidity.py 139 0 56 0 100% src/CSET/operators/imageprocessing.py 56 0 16 0 100% src/CSET/operators/mesoscale.py 17 0 2 0 100% src/CSET/operators/misc.py 172 1 74 4 98% src/CSET/operators/plot.py 1110 166 388 71 82% src/CSET/operators/power_spectrum.py 97 3 30 3 95% 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 433 22 190 18 93% src/CSET/operators/regrid.py 146 1 70 3 98% src/CSET/operators/scoreswrappers.py 160 22 42 8 82% src/CSET/operators/temperature.py 121 0 32 0 100% src/CSET/operators/transect.py 62 0 24 0 100% src/CSET/operators/wind.py 45 3 10 2 91% src/CSET/operators/write.py 15 0 6 0 100% src/CSET/recipes/__init__.py 104 0 28 0 100% --------------------------------------------------------------------------------------------------- TOTAL 5131 319 1762 156 92% |
some example plots |
|
This is currently working with a netcdf cube that already has hourly precipitation. Challenge now will be loading .pp files into cset and then calculating hourly precip from them. dFSS will need a science review - especially the modified improver code. |
|
David Flack (@daflack) , I removed the loaders stuff for now so this PR can just be focused more on the operator and plots (hopefully making it simpler to review) I will make another issue for plumbing in the workflow etc after |
|
Hi James, Thank you for your work on this. Sorry for taking so long to get to the review. I'm going to do this review in two parts. First I'm mostly looking at the neighbourhood method that is based on the improver code. I'm happy with what the neighbourhood code is trying to do and how it copes with the edges by masking the values that are not to be included to create the sum and calculating a mean based on the number of valid points only. However, I think the neighbourhood length is not set correctly. There are two relevant length scales: (ii) the diameter or neighbourhood length/size, which is defined as the whole length of the neighbourhood, so the above would have a neighbourhood length/size of 3. A radius can be any integer and the neighbourhood length must be an odd number. In the line in the above comment, it looks like the nb_size is set as the radius. This will cause problems in some of the other routines. I'm surprised that it works if a neighbourhood length of 2 is passed in to the neighbourhooding routine. I've suggested a change to this. Could you have a look and see whether you agree? And perhaps we could have some simple tests that just check the neighbourhooding is working as we expect? Is the neighbourhood_length in the dFSS calculations intended to be the radius or the length/size? If it's the radius, then perhaps we should rename is as neighbourhood_radius so that it is clear which one we are using. Happy to be told that I'm mistaken if I've missed something in the code. |
|
You're right, and this is a bit confusing. Originally in your code we were dealing with the odd valued neighbourhood lengths, however the input for the original improver neighbourhood is a list of radii which is then converted to odd valued neighbourhood length. (see here: https://github.com/metoppv/improver/blob/8b7dce99c0eabff19e7119ed3696ef630efd8bb5/improver/nbhood/nbhood.py#L445) Because we have been working with neighbourhood lengths previously, I basically skipped this conversion so that we could just input neighbourhood lengths into improver (albeit I didn't change the variable name from radii -> neighbourhood lengths so it is confusing ). see: CSET/src/CSET/operators/improver/nbhood.py Lines 521 to 527 in 7f50975 I could add the conversion back of course and we could work with radii at the top level instead. |
|
Thanks James Ross (@james-a-f-ross). With the code as it is, then the circular neighbourhood calculation will be wrong as it is expecting a radius rather than a neighbourhood length. I know we're not using this functionality at the moment, but if we've got it in there, it should be correct. So I think either switch to using radii at the input level, or remove/adjust the circular neighbourhood code. I don't mind which as long as it is clear in the gui and consistently named in the code. Probably neighbourhood size is more intuitive - in which case, a check that it is an odd number early on would be useful (and just re-calculate the radii for the circular neighbourhood routine only). The other confusing thing is that in tests/operators/test_dfss.py, you use neighbourhood lengths that are not odd. Is this to make sure they fail? I'll continue with the rest of the review now. |
|
I've had a look at src/CSET/operators/dfss.py and I'm happy with most of the science. One thing I'm not sure about is whether we still need to regrid to the xy grid. I had to do this originally to run the improver code, but I don't think this check is in place anymore. Could you do a quick check to see whether the code runs without the regrid, and if it produces similar results? If we do still need it, then can you have a look at where you are using cube_xy and make sure that it is actually a regridded cube at that point. I don't think it is as you are now regridding when you are calculating the FSS. In my code, the regrid was done before the dFSS calc so that is why the routines have cube_xy as input. It's just to make the code clearer. Also, when you are regridding, the ensemble member is now irrelevant so you can probably drop this from the regrid code. Is it faster or slower to do the regrid each time in this way? I would have thought it would be quicker to just do it once and then pass it through. I don't know for sure though. |
annemccabe
left a comment
annemccabe
left a comment
There was a problem hiding this comment.
Thanks for all of this James. Just a few things to address. This is a big change though so really pleased that it's so close to going on the trunk.
| title: dFSS | ||
| description: | | ||
| runs the dFSS calculation then creates: | ||
| 1) line plots with dfss vs neighbourhoods for each time coordinate |
There was a problem hiding this comment.
Could we also have plots of dfss vs time for each neighbourhood please.
And it would be nice for the line plots to use the stdev as error bars on the dfss plots - so you could shade the region 1 standard deviation above and below the mean.
|
annemccabe have also implemented checks in the code for neighbourhood_length validity, and when it comes to the gui will need to tell users what values are valid for this. I am guessing we want neighbourhood_length = 1 to be invalid here, since then grid_cells is 0. |
# Conflicts: # pyproject.toml # src/CSET/operators/plot.py
Co-authored-by: annemccabe <43886422+annemccabe@users.noreply.github.com>
06b9148 to
afe88d7
Compare
|
Nigel Roberts (@NigelRoberts) just tagging you into this PR as it is our implementation of dFSS. Happy to take advice. The neighbourhooding is done with slightly altered improver code, such that the input is the length of neighbourhoods (e.g 3,5,7...) rather than radius. |
3 participants
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.