-
Notifications
You must be signed in to change notification settings - Fork 8
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
Issue 99 #120
Issue 99 #120
Changes from all commits
4feb03b
ae63067
573ef66
1fa665f
eb987cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,8 +144,17 @@ def _regrid_array(self, source): | |
source = source.reshape((-1, source_grid.size)) | ||
|
||
size = self._target.size | ||
|
||
if isinstance(source, DaskArray): | ||
chunks = source.chunks[: -source_grid.ndim] + (self._target.shape,) | ||
# It's possible that the topology dimensions are chunked (e.g. from | ||
# reading multiple partitions). The regrid operation does not | ||
# support this, since we might need multiple source chunks for a | ||
# single target chunk, which destroys the 1:1 relation between | ||
# chunks. Here we ensure that the topology dimensions are contained | ||
# in a single contiguous chunk. | ||
contiguous_chunks = (source.chunks[0], (source.shape[-1],)) | ||
source = source.rechunk(contiguous_chunks) | ||
chunks = source.chunks[:-1] + (self._target.size,) | ||
out = dask.array.map_blocks( | ||
self._regrid, # func | ||
source, # *args | ||
|
@@ -162,7 +171,6 @@ def _regrid_array(self, source): | |
"Expected dask.array.Array or numpy.ndarray. Received: " | ||
f"{type(source).__name__}" | ||
) | ||
|
||
# E.g.: sizes of ("time", "layer") + ("y", "x") | ||
out_shape = first_dims_shape + self._target.shape | ||
return out.reshape(out_shape) | ||
|
@@ -203,12 +211,7 @@ def regrid(self, object) -> UgridDataArray: | |
if type(self._target) is StructuredGrid2d: | ||
source_dims = ("y", "x") | ||
regridded = self.regrid_dataarray(object, source_dims) | ||
regridded = regridded.assign_coords( | ||
coords={ | ||
"y": np.flip(self._target.ybounds.midpoints), | ||
"x": self._target.xbounds.midpoints, | ||
} | ||
) | ||
regridded = regridded.assign_coords(coords=self._target.coords) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the y coordinates still flipped in this implementation? It is important to have y coordinates in descending order I see you defined a new property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the coords property to the StructuredGrid- class. There it uses the actual grid properties directly and not the possible flipped midpoints. This implementation was needed to get the right dx/dy from the target-grid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, just to double check: This doesn't result in grids which are flipped from descending There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it does not indeed. Only the midpoints arrays are fliped for computing the proper weights. The original coords are untouched and now used. I double checked myself + it is also tested in the testbench |
||
return regridded | ||
else: | ||
source_dims = (object.ugrid.grid.face_dimension,) | ||
|
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.
Why are you calling the reshape in the if else tree above, instead of at the end like it was? The original version appears to be cleaner to me, as
reshape
only has to be mentioned once, instead of twice after your alterations.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.
Because the reshape does not work for a dask-array. For the dask-array I first used the dask.array.reshape(), which gives the result that we want. For the current implementation (see reply above) I could move it back indeed.
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.
Ah an understandable struggle with
dask.array.reshape
:-) I would prefer if you moved these lines back down because we can leave the code as it was now that the call todask.array.compute
was introduced.