CMUG Sea Surface Salinity dataset and diagnostic #1832
Conversation
|
|
||
| Recipes are stored in recipes/ | ||
|
|
||
| * recipe_sea_surface_salinity |
There was a problem hiding this comment.
| * recipe_sea_surface_salinity | |
| * recipe_sea_surface_salinity.yml |
| Variables | ||
| --------- | ||
|
|
||
| * sos (ocean, monthly mean, time depth_id) |
There was a problem hiding this comment.
| * sos (ocean, monthly mean, time depth_id) | |
| * sos (ocean, monthly, longitude, latitude, time) |
There was a problem hiding this comment.
This should refer to the original data or the dimensions expected by the diagnostic after the preprocessor?
There was a problem hiding this comment.
So far, this ususally referred to the dimensions expected by the recipe (recipe as provided in the official release). I guess we should think about how to handle this in the future and make it more clear to what this actually refers. Or maybe simply get rid of this section? What do people think?
| @@ -0,0 +1,86 @@ | |||
| .. _recipes_sea_surface_salinity: | |||
|
|
|||
| Sea Surface Salinity Evaluation | |||
There was a problem hiding this comment.
I think it would be good to have a section in the documentation on:
(1) how to obtain the shapefile needed
(2) in which directory to put the shapefile
(3) available regions (or at least examples) for analysis
(4) required / recommended preprocessor settings (extract_shape)
|
I think I addresed all doc related comments. Anyway, this can not be merged because there is a PR in the core that is still pending |
|
Both, ESMValGroup/ESMValCore#764 and ESMValGroup/ESMValCore#798 have been merged. Is there anything else missing to move forward with this PR? |
|
I just tried to test this recipe again with the latest ESMValCore (master) but had no success. The first problem was with the preprocessor area_statistics: After changing to the recipe starts but then crashes again with @jvegasbsc Could you please take a look? Are there other things that still need to be done / adressed before merging? It would be good to get things moving soon so we can meet our CMUG deliverables. |
|
The preprocessor defined in recipe_sea_surface_salinity.yml creates time series by extracting some given shapes and calculating area means. Each of these two operations works individually but not when combined. As the diagnostic was working before, the error is probably related to changes in the preprocessor that have been introduced during the last year. I could not figure out what might go wrong. @valeriupredoi could you maybe take a look? We need to get this merged soon to meet our CMUG project deliverables and I would greatly appreciate some help as @jvegasbsc does not seem to be around any more... |
|
hi @axel-lauer could you please merge the latest |
|
Thanks @valeriupredoi ! I just merged the latest main into this branch. Here is the log file from running the recipe: main_log_debug.txt |
|
cheers @axel-lauer - am having a looksee now 👍 |
|
@axel-lauer here's what I found:
-> the reason why there are 9 is that there are 9 regions to be selected and merged into one big map, but the problem is that they are not time-dependent, and that suggests to me that those are the cell measures cubes rather than the actual data cube masked. Here is the issue ESMValGroup/ESMValCore#1394 |
|
Big thanks to @sloosvel for opening PR ESMValGroup/ESMValCore#1403 This seems to fix the problem with extract_shape, i.e. the preprocessor finishes now successfully! That's really cool! |
axel-lauer
added
approved by scientific reviewer
and removed
in scientific review
labels
|
cheers @axel-lauer 🍺 Lemme have a looksee in a very short bit (10min) and will approve and merge 👍 |
| reference_dataset = variables.pop(ref_alias)[0] | ||
| reference = iris.load_cube(reference_dataset[n.FILENAME]) | ||
| reference_ancestor = reference_dataset[n.FILENAME] | ||
| logger.debug(reference) |
There was a problem hiding this comment.
please add a verbose message to debugger, not just a cube
There was a problem hiding this comment.
Added more verbose message...
| time_coord.units.name, | ||
| calendar='gregorian', | ||
| ) | ||
| unify_time_units((reference, dataset)) |
There was a problem hiding this comment.
will this always succeed? Probably best to catch an error with a try/except statement
There was a problem hiding this comment.
Not sure why @jvegasbsc implemented this. I would leave it as is since it seems to be working fine...
| calendar='gregorian', | ||
| ) | ||
| unify_time_units((reference, dataset)) | ||
| logger.debug(dataset) |
There was a problem hiding this comment.
here too please add a user-readable message
There was a problem hiding this comment.
Added more verbose message...
| 'caption': caption, | ||
| 'domains': ['global', ], | ||
| 'autors': ['vegas-regidor_javier'], | ||
| 'references': ['acknow_author'], |
There was a problem hiding this comment.
I believe there is none. @jvegasbsc is that right or would you have a suggestion?
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
OK - I am bit confused why you've created a couple very hefty objects instead of just using functions - it's not like the diagnostic is a module that gets used many times
There was a problem hiding this comment.
Not sure, but I would guess @jvegasbsc had a good reason...
valeriupredoi
added
in technical review
and removed
looking for technical reviewer
labels
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Replaced ^^^ with ---
There was a problem hiding this comment.
cheers @axel-lauer 🍺 It still needs a bit of polishing but I believe you guys need this in asap, so I'm not gonna be the proverbial cork in the bottle especially the bits that are still needed are minor 😁
valeriupredoi
added
approved by technical reviewer
and removed
in technical review
labels
|
looks good, merge at will @axel-lauer 👍 |
|
Big thank you @valeriupredoi for your quick help!! I owe you a 🍺 next time we can meet in person! |
|
@axel-lauer glad I could help! I won't say no to the 🍺 tho 😁 |
Add ESACCI-SEA-SURFACE-SALINITY dataset (v1 and v2) and a diagnostic showing how to
Requires ESMValGroup/ESMValCore#764 and ESMValGroup/ESMValCore#798
Tasks
yamllintto check that your YAML files do not contain mistakesNew recipe/diagnostic
doc/sphinx/source/recipesfolder and add a new entry to index.rstNew data reformatting script