Load fx variables as cube cell measures / ancillary variables #999
Conversation
There was a problem hiding this comment.
Very nice work @sloosvel, it would be great if you could make the pull request ready for review!
esmvalcore/preprocessor/_other.py
Outdated
| mip = loaded_cube[0].attributes['table_id'] | ||
| freq = loaded_cube[0].attributes['frequency'] |
There was a problem hiding this comment.
It might be better to pass these in as function arguments rather than relying on the input NetCDF files to have these global attributes
|
nice work @sloosvel - could you please address the Codacy complaints? I will have a good look after that, still stuck in Tool-land at the moment 👍 |
There was a problem hiding this comment.
Thanks, I think this is a very nice new feature. Could you please have a look at the comments?
Could you also add documentation for the new preprocessor function add_cell_measure to doc/recipe/preprocessor.rst ?
Are you planning to add another very similar preprocessor function for adding other ancillary variables in a follow up pull request? E.g. land-sea masks for the masking preprocessor functions?
esmvalcore/preprocessor/_area.py
Outdated
| return grid_areas | ||
|
|
||
|
|
||
| # get the area average | ||
| def area_statistics(cube, operator, fx_variables=None): |
There was a problem hiding this comment.
Can the fx_variables argument now be removed from this function, since they are now available in the cube?
There was a problem hiding this comment.
I thought of that, but then how would someone specify in the recipe the fx file that is needed?
| @@ -388,7 +391,11 @@ experiment is preferred for fx data retrieval: | |||
| landmask: | |||
| mask_landsea: | |||
| mask_out: sea | |||
| fx_variables: [{'short_name': 'sftlf', 'exp': 'piControl'}, {'short_name': 'sftof', 'exp': 'piControl'}] | |||
| fx_variables: | |||
There was a problem hiding this comment.
I like the new syntax, but could you make it backward compatible so the old syntax keeps working, at least for a while?
There was a problem hiding this comment.
Maybe until version 2.4 or 2.5?
There was a problem hiding this comment.
I can try, but it won't look too good I think.
There was a problem hiding this comment.
yesh, I was asking a related question in my previous comment. I agree with @bouweandela on this one, when a new functional feature is implemented it's always nice to have a temporary overlap with its previous syntax 👍
esmvalcore/preprocessor/__init__.py
Outdated
| @@ -41,7 +41,7 @@ | |||
| mask_outside_range, | |||
| ) | |||
| from ._multimodel import multi_model_statistics | |||
| from ._other import clip | |||
| from ._other import clip, add_cell_measure | |||
There was a problem hiding this comment.
it's added in _other.py, but that may not be the best option).
Maybe you could add a new module esmvalcore/preprocessor/_ancillary_variables.py and keep the code there? I feel that you are right and other is not very clear.
esmvalcore/preprocessor/_volume.py
Outdated
| if cube.data.ndim == 4 and grid_volume.ndim == 3: | ||
| grid_volume = np.tile(grid_volume, | ||
| [cube_shape[0], 1, 1, 1]) | ||
| if not fx_variables: |
There was a problem hiding this comment.
Same for this function, I think the fx_variables argument can be removed now, right?
|
@sloosvel I am very happy for this PR to be merged asap, I have gone through it in detail and I reckon it's a very nice one and you've answered all my queries (cheers for being patient with me!). The only thing preventing a merge is the broken test on Circle that is not broken in reality, I'd like @bouweandela to shed light on this via #1063 (when he's got time) and then we'll get this in right away 🍺 |
@valeriupredoi The test is fixed now, let's get this merged if you think it's ready! If needed, we can continue the discussion on the tests in the issue. I won't be back at work for another two weeks, let's not wait for that. |
|
Actually I have been wondering if it would be better to remove the ancillary vars/ measures after the preproc steps that need them finish. |
|
@bouweandela cheers! @sloosvel all fine and ready for merge by me, just let me know if you think this is ready-ready 👍 BTW I am totally fine with the Codacy issues too 🍺 |
|
@sloosvel I think that would be better, maybe just before the |
|
good to merge @sloosvel ? I feel the need for speed(merge) 😁 |
|
Well it seemed to work for what I have tested but if anyone else wants to double check they are welcome to! |
|
I am very happy to merge now and fix whatevs later, I tested this and it works - if there are any corner cases we can always open new PRs, this is great amount of work and added functionality that needs not wait no more 👍 |
|
thank you very very much again @sloosvel 🍺 |
Description
This pull requests adds a new default preprocessor step called
add_cell_measures. This step cmor-checks and loads areas and volumes as cell measures into a cube variable if the preprocessor stepsarea_statisticsandvolume_statisticsare called in the recipe. Thus allowing the preprocessor chain to be applied in fx variables. If those steps are not included, nothing happens to the original cube.It should also solve a couple of issues that there are with the current fx handling:
fx_variablesoption as both a list and a list of dicts. Instead this pull requests proposes to call the fx_variables using YAML-file identations so that when the recipe is loaded, it is always loaded as a dict. Because allowing both lists and dicts ends up complicating the the code.I open this as a draft because there may be some issues in terms of where to define this preprocessor step (it's added in
_other.py, but that may not be the best option). Also because I had to modify code that I did not originally write, so even though tests are passing, maybe I changed something that should not have been changed.Before you get started
Checklist
pre-commitoryamllintchecksIf you make backwards incompatible changes to the recipe format:
To help with the number pull requests: