-
Notifications
You must be signed in to change notification settings - Fork 44
Add custom CMOR table and derivation script for permafrost #2358
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2358 +/- ##
==========================================
+ Coverage 95.59% 95.61% +0.01%
==========================================
Files 266 267 +1
Lines 15573 15629 +56
==========================================
+ Hits 14887 14943 +56
Misses 686 686 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
In order to maintain a backlog of relevant pull requests, we automatically label them as stale after 180 days of inactivity. If this pull request is still important to you, please comment below to remove the stale label. Otherwise, this pull request will be automatically closed in 60 days. If this pull request only suffers from a lack of reviewers, please tag the @ESMValGroup/technical-lead-development-team so they can help you find a suitable reviewer. |
bouweandela
left a comment
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 looks like there is also a derivation function in this pull request that is not mentioned in the description at the top. Is that intentional? If yes, please make sure that the derivation is lazy and add a unit test for it.
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
|
If you click the |
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
… into esacci-permafrost
| # loop over all years and test if frost is present throughout | ||
| # the whole test period, i.e. [year-test_period, year+test_period] | ||
|
|
||
| for tidx, year in enumerate(year_coord.points): | ||
| # extract test period | ||
| pdt1 = PartialDateTime( | ||
| year=year - 1, | ||
| month=13 - test_period, | ||
| day=1, | ||
| ) | ||
| pdt2 = PartialDateTime(year=year + 1, month=test_period + 1, day=1) | ||
| daterange = iris.Constraint( | ||
| time=lambda cell, pdt1=pdt1, pdt2=pdt2: pdt1 | ||
| <= cell.point | ||
| < pdt2, | ||
| ) | ||
| soiltemp_window = soiltemp.extract(daterange) | ||
| # remove auxiliary coordinate 'year' to avoid lots of warnings | ||
| # from iris | ||
| soiltemp_window.remove_coord("year") | ||
| # calculate mean over test period | ||
| test_cube = soiltemp_window.collapsed("time", iris.analysis.MEAN) | ||
| # if all months in test period show soil tempeatures below zero | ||
| # then mark grid cell with "1" as permafrost and "0" otherwise | ||
| pfr_yr.data[tidx, :, :] = da.where(test_cube.data > 0.99, 1, 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.
Could this be replaced by the rolling_window_statistics preprocessor function? Something like:
pfr_yr = esmvalcore.preprocessor.rolling_window_statistics(
soiltemp,
coordinate="time",
operator="mean",
window_length=FROZEN_MONTHS,
)
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.
Not sure. I tried a few options but could not reproduce the original output. Not sure it is worth spending much more time on this possible replacement.
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 rolling_window_statistics could be used if you extend the input cube by test_period time steps to both sides (the rolling window with a window > 1 will produce a shorter output cube; this accounts for that). Not sure if that's makes the code simpler, probably not...
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 looks like the current implementation creates fairly small dask chunks, which may lead to slow/inefficient computations. Every time you create a dask array and use it to build another dask array, you insert all the dask chunks needed to build that subarray into the final array. Looping over the time-axis and computing things separately for each time step is a typical example of this. This may be aggravated by the use of aggregated_by SciTools/iris#5455. Therefore, I think it would be worth investigating if rolling_window_statistics can be used to reduce the size of the Dask task graph and increase the chunk sizes.
However, maybe this works just fine, it's hard to tell from just looking at the code. Did you test the runtime? Can you run the computation using the distributed scheduler? And did you test how long it takes to compute a multi-model mean over e.g. 100 datasets?
It would be nice to have a good unit test, so we can look at improving computational performance later https://github.com/ESMValGroup/ESMValCore/pull/2358/files#r2690416067 if needed.
| out_cube = derived_var.calculate(in_cubes) | ||
| assert out_cube.units == cf_units.Unit("%") | ||
| out_data = out_cube.data | ||
| expected = 100.0 * np.ones_like(out_cube.data) |
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 would be nice to have a test case where not the entire cube is permafrost because this doesn't really check that the calculation is correct.
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
|
Hello, this pull request has been marked with the If you won't be able to finish this in time, don't worry - just unassign the milestone |
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2358 +/- ##
==========================================
+ Coverage 95.59% 95.61% +0.01%
==========================================
Files 266 267 +1
Lines 15573 15629 +56
==========================================
+ Hits 14887 14943 +56
Misses 686 686 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR adds (a) custom CMOR tables for the following three ESACCI-PERMAFROST variables:
and (b) a derivation script for permafrost extent (pfr) to calculate this variable from the CMIP models.
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
- [ ] 🧪 and 🛠 Documentation is available- [ ] 🛠 Any changed dependencies have been added or removed correctly- [ ] 🛠 The list of authors is up to date