Added new Extract surface preprocessor #1048
Conversation
ledm
added
enhancement
valeriupredoi
left a comment
valeriupredoi
left a comment
There was a problem hiding this comment.
@ledm cheers dude! See a couple comments from me 👍 🍺
esmvalcore/preprocessor/_volume.py
Outdated
| elif zcoord.points.ndim == 3: | ||
| points = zcoord.points.mean(axis=[1, 2]) | ||
| elif zcoord.points.ndim == 4: | ||
| points = zcoord.points.mean(axis=[0, 2, 3]) |
There was a problem hiding this comment.
could zcoord.points.ndim == 2?
There was a problem hiding this comment.
either way, maybe add an exception if any of the dims are not the ones you analyze
There was a problem hiding this comment.
You're right.
The problem with that (and this method as a whole) is that it makes assumptions about the z-axis position. For instance, what if you have a cube with dimensions (time, depth, latitude) ie a meridional transect? The preprocessor would fail.
Maybe instead of that, we could replace this if-else loop with something more generic?
zcoord_dim = cube.coord_dims(zcoord)[0]
axes = np.arrange(zcoord.points.ndim).pop(zcoord_dim)
points = zcoord.points.mean(axis=axes)
| return cube[:, surf] | ||
|
|
||
| logger.error('Condition not recognised: postive: %s , zcoord_dim: %s, ' | ||
| 'surface level: %s', positive, zcoord_dim, surf) |
There was a problem hiding this comment.
this is a very cryptic exception, could you reformulate for the user to understand what actually is wrong pls?
| result = extract_surface(self.grid_3d) | ||
| self.assert_array_equal(result.data, self.grid_2d.data) | ||
| result2 = extract_surface(self.grid_4d) | ||
| self.assert_array_equal(self.grid_4d[:, 0, :, :].data, result2.data) |
There was a problem hiding this comment.
do you have a case for 1D as well?
Co-authored-by: Valeriu Predoi <[email protected]>
Co-authored-by: Valeriu Predoi <[email protected]>
Co-authored-by: Valeriu Predoi <[email protected]>
Co-authored-by: Valeriu Predoi <[email protected]>
|
@ledm a couple linting issues and a couple of me comments still to be looked at, nice progress BTW 👍 🍺 |
|
Ok, let's focus on other stuff for the upcoming release then. |
Conflicts: esmvalcore/cmor/_fixes/cmip6/bcc_csm2_mr.py esmvalcore/cmor/_fixes/cmip6/cesm2_fv2.py esmvalcore/cmor/_fixes/cmip6/cesm2_waccm.py esmvalcore/cmor/_fixes/cmip6/cesm2_waccm_fv2.py
|
This still contains very many fixes. @ledm, I'd love to see this preprocessor go in, but there are just too many unrelated things here. Could you please separate out the fixes so we can move forward? Let me know if you need git help or something. |
Conflicts: esmvalcore/_data_finder.py esmvalcore/_recipe.py
|
I'll bump this to the next version. If we can't make any progress by then (Feb 2022), I'll suggest dropping this. |
|
@ledm do you have any intentions to further work on this for v2.5? |
|
I have a conceptual problem with this. I think it shouldn't be it's own thing, but rather should be a special case of the extract_levels preprocessor. I don't think users should have to know the difference... |
|
I agree with that! Do you think this is something you can do for v2.5? If not I will remove the milestone label so that we have a nice overview of what needs to be done before the release. 👍 |
|
I agree and I think probably we won't make it for 2.5.0. I would move it to 2.6.0 instead of just removing the milestone. |
|
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. |
|
This stale pull request has been automatically closed due to a lack of community activity. If you still care about this pull request, please re-open it. If you do not have sufficient permissions for this, tag the @ESMValGroup/technical-lead-development-team in a comment below including a short message that this pull request is still relevant to you. They will re-open it on your behalf. |
Description
This PR adds the
extract_surfacepreprocessor, this is necessary because of the issues described in #1039.Basically, when we're looking at a 3D dataset, but only want to keep the surface layer. Extract_layer can be used to do this, but it actually regrids the entire dataset along the z axis, so this is incredibly slow and memory intensive.
Before you get started
Checklist
pre-commitoryamllintchecksIf you make backwards incompatible changes to the recipe format:
To help with the number pull requests: