-
Notifications
You must be signed in to change notification settings - Fork 39
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
Global Total Integrated primary production derived variable #270
Conversation
@@ -107,15 +107,22 @@ def _put_in_cube(template_cube, cube_data, statistic, t_axis): | |||
t_axis, | |||
standard_name='time', | |||
units=template_cube.coord('time').units) | |||
lats = template_cube.coord('latitude') | |||
lons = template_cube.coord('longitude') | |||
|
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.
These changes seem unrelated to deriving the variable? Would it make sense to put this in a separate pull request?
|
||
try: | ||
cube_area = cubes.extract_strict(iris.Constraint(name='cell_area')) | ||
except iris.exceptions.ConstraintMismatchError: |
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 this happens, cube_area
will be undefined and the code below will fail, so I think it is not a good idea to let this pass
iris.analysis.MEAN, | ||
) | ||
|
||
# TODO: Load seconds per year from model calendar. |
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 your to do?
times = intpp_cube.coord('time') | ||
|
||
intpp_cube.data = np.ma.array(intpp_cube.data) | ||
for time_itr in np.arange(len(times.points)): |
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.
Would it be possible to use iris.analysis.SUM
to aggregate over the required dimensions?
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 you please add a unit test too?
@ledm can you finalize this PR so that I can test it? |
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.
Following the logic of #331, is this now redundant? This could be specified in a recipe using a combination of fx weighting, the area information and the sum preprocessor?
This PR can thus be closed?
The fact that the derived variable's units lose the per m2 is related to the comment here #1574 (comment) In other words, the fact that a sum weighted by area does not include units (i.e. by multiplying the units by area ** 2) is an ongoing issue (issue now open at #1613 to track) |
I think this can now be done with the following preprocessor: area_statistics:
operator: sum (including correct unit handling). Please re-open if necessary. |
This PR brings in the Global Total Integrated primary production derived variable. This variable has appeared in the upcoming ESMValTool Paper - part 2, so it would be nice to get it out there before the publication gets to discussions format.
This is linked with the ESMValTool PR ESMValGroup/ESMValTool#1331, which uses this derived field.