Remap ICON to swiss #44
Conversation
src/meteodatalab/operators/regrid.py
Outdated
| return indices, weights | ||
|
|
||
|
|
||
| def _cropped_domain(xy, uv, buffer=1000): |
There was a problem hiding this comment.
I think it would make sense to provide types and a bit more doc on these functions.
Even if declare private, for maintainability and increase its readability by developers.
Arguments are passed to other internal functions and complexity is not so trivial anymore.
There was a problem hiding this comment.
Also the function is not just cropping the domain, since it is also computing the weights?
There was a problem hiding this comment.
Yes, it's essentially cropping the input domain before passing it to the computation of the weights to avoid creating triangles where no interpolation is done.
| return idx[indices], weights | ||
|
|
||
|
|
||
| def iconremap( |
There was a problem hiding this comment.
maybe a comment on the API:
we have
- icon2geolatlon: implicit method (RBF), dst grid can not be passed
- icon2rotlatlon: implicit method (RBF), dst grid can not be passed
- iconremap: method passed as argument (byc), dst grid passed by argument
could we homogenise the api, maybe also 1 single entry point that dispatches according to the method?
There was a problem hiding this comment.
I don't think that multiplexing the function through a single interface is a good design. Especially in this case where the behavior change between the methods is so large. On the one hand, the weights are pre computed so the destination grid is dictated by that and on the other the weights are computed on the fly so a destination grid can be defined.
There was a problem hiding this comment.
If you are not considering refactoring the api, I would at least make it explicit that pre-computed weights from RBF are used, either through docstrings or by renaming those methods.
src/meteodatalab/operators/regrid.py
Outdated
| def iconremap( | ||
| field: xr.DataArray, dst: RegularGrid, method: Literal["byc"] = "byc" | ||
| ) -> xr.DataArray: | ||
| """Remap ICON native grid data to the swiss grid. |
There was a problem hiding this comment.
Is it necessarily the swiss grid?
There was a problem hiding this comment.
No it isn't. I'll change this
| dst : RegularGrid | ||
| A regular grid in the swiss coordinate system. | ||
| method : Literal["byc"] | ||
| Method used to perform the interpolation. |
There was a problem hiding this comment.
Missing description of the interpolation method.
src/meteodatalab/operators/regrid.py
Outdated
| xy = np.array(points_src).T | ||
| uv = np.array(points_dst).T | ||
|
|
||
| if method == "byc": |
There was a problem hiding this comment.
I would simply put
if method not in ["byc"]:
raise NotImplementedError(f"method: {method} is not implemented")at the start of the function.
There was a problem hiding this comment.
I was hoping to add more than one but that would be for another PR
Purpose
Provide a single step interpolation operator from ICON native grid to regular grids in the Swiss coordinate systems.
Note that technically any regular grid in a projected, Cartesian coordinate system is admissible but warning is emitted if the coordinate system is not Swiss.The interpolation is done on a UTM that is expected to be usable over the entire ICON domain and thus any relevant output CRS can be used (rotlatlon, geolatlon, swiss, etc...)Note that the unit is disabled due to the fieldextra implementation taking too much time to execute.
Code changes:
regrid.iconremapoperator.Requirements changes: