Skip to content
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

Test if xugrid's and iMOD Python's Regridder behave similar enough #1372

Closed
JoerivanEngelen opened this issue Jan 8, 2025 · 6 comments · Fixed by #1463
Closed

Test if xugrid's and iMOD Python's Regridder behave similar enough #1372

JoerivanEngelen opened this issue Jan 8, 2025 · 6 comments · Fixed by #1463
Assignees
Milestone

Comments

@JoerivanEngelen
Copy link
Contributor

It would be good to test iMOD Python's regridder and xugrid's behave similar enough.
If there are any noticeable differences these should be documented.

@Huite
Copy link
Contributor

Huite commented Jan 13, 2025

The most important thing is that the xugrid one is missing the Voxelizer and LayerRegridder, essentially.

This requires top and bottom data, or bounds on a z coordinate. I've implemented the logic for computing the weights: https://github.com/Deltares/xugrid/blob/fc0bdcb0eaeff62887f1e62fadd61957629dff61/xugrid/regrid/overlap_1d.py#L158

But it's not exposed yet here (class exists: https://github.com/Deltares/xugrid/blob/fc0bdcb0eaeff62887f1e62fadd61957629dff61/xugrid/regrid/structured.py#L736)

@JoerivanEngelen JoerivanEngelen self-assigned this Feb 26, 2025
@JoerivanEngelen JoerivanEngelen moved this from 📯 New to 🏗 In Progress in iMOD Suite Feb 26, 2025
@JoerivanEngelen
Copy link
Contributor Author

I just modified some unittests of the legacy regridder to verify if results are the same, which they are. Furthermore I did a simple performance test and it seems xugrid's is faster:

# %%
import imod
import xugrid as xu
import numpy as np

xmin = 0.0
xmax = 1000.0
ymin = 0.0
ymax = 1000.0
dcell_src = 1.0
dcell_dst = 2.5
layer = np.arange(20) + 1

src = imod.util.empty_3d(dcell_src, xmin, xmax, dcell_src, ymin, ymax, layer)
like = imod.util.empty_2d(dcell_dst, xmin, xmax, dcell_dst, ymin, ymax)

rng = np.random.Generator(np.random.PCG64(12345))
src[:, :, :] = rng.random(src.shape)

# %%
dst = imod.prepare.Regridder(method="mean", use_relative_weights=True).regrid(src, like)

# %timeit prints on my machine:
# 1.58 s ± 10.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

# %%
dst_new = xu.OverlapRegridder(source=src, target=like, method="mean").regrid(src)

# %timeit prints on my machine:
# 90.9 ms ± 1.38 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

@Huite
Copy link
Contributor

Huite commented Feb 26, 2025

That speedup would be satisfying!

Note that both rely on Numba so there may be some JIT-overhead.

The xugrid regridder should be a lot more memory hungry since it's computing and storing all the weights.
On the other hand, the imod regridder recomputes the same weights over and over (just multiplying floats).

@JoerivanEngelen
Copy link
Contributor Author

For completeness, I also tested this (continuation of previous script), xugrid wins again:

...
# %%
regridder = imod.prepare.Regridder(method="mean", use_relative_weights=True)
# %%
dst = regridder.regrid(src, like)
# %timeit prints on my machine:
# 393 ms ± 5.68 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

# %%
regridder_new = xu.OverlapRegridder(source=src, target=like, method="mean")
# %%
dst_new = regridder_new.regrid(src)
# %timeit prints on my machine:
# 15.9 ms ± 285 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

@JoerivanEngelen
Copy link
Contributor Author

JoerivanEngelen commented Feb 27, 2025

I noticed some slight differences in the location of NaN values when downscaling with the BarycentricInterpolator. It seems the BarycentricInterpolator activates one line of cells more than imod.prepare.Regridder. I prefer this over the previous behavior of the BarycentricInterpolator where it deactivated more cells than the imod.prepare.Regridder. It is probably something to document though.

I see for safety, iMOD Python now masks everything with the smallest possible idomain based on all regridders, so then xu.OverlapRegridder determines what is active or not instead of xu.BarycentricInterpolator. (Granted that xu.CentroidLocator is not used).

# %%
import imod
import xugrid as xu
import numpy as np

xmin = 0.0
xmax = 1000.0
ymin = 0.0
ymax = 1000.0
dcell_fine = 1.0
dcell_coarse = 2.5
layer = np.arange(20) + 1

# %%
## TEST CASE: UPSCALING GRID WITH SQUARE OF NAN CELLS IN CENTER
src = imod.util.empty_3d(dcell_fine, xmin, xmax, dcell_fine, ymin, ymax, layer)
like = imod.util.empty_2d(dcell_coarse, xmin, xmax, dcell_coarse, ymin, ymax)

rng = np.random.Generator(np.random.PCG64(12345))
src[:, :, :] = rng.random(src.shape)
src[:, 400:600, 400:600] = np.nan

# %%
dst = imod.prepare.Regridder(method="mean", use_relative_weights=True).regrid(src, like)
dst_new = xu.OverlapRegridder(source=src, target=like, method="mean").regrid(src)
# Test if nodata values are at the same location
assert not np.any(np.isnan(dst_new) != np.isnan(dst))

# %%
dst_new = xu.BarycentricInterpolator(source=src, target=like).regrid(src)
# Test if nodata values are at the same location
assert not np.any(np.isnan(dst_new) != np.isnan(dst))

# %%
## TEST CASE: DOWNSCALING GRID WITH SQUARE OF NAN CELLS IN CENTER
src = imod.util.empty_3d(dcell_coarse, xmin, xmax, dcell_coarse, ymin, ymax, layer)
like = imod.util.empty_2d(dcell_fine, xmin, xmax, dcell_fine, ymin, ymax)

rng = np.random.Generator(np.random.PCG64(12345))
src[:, :, :] = rng.random(src.shape)
src[:, 150:250, 150:250] = np.nan

# %%
dst = imod.prepare.Regridder(method="multilinear", use_relative_weights=True).regrid(src, like)
dst_new = xu.OverlapRegridder(source=src, target=like, method="mean").regrid(src)
assert not np.any(np.isnan(dst_new) != np.isnan(dst))
# Doesn't fail, so NaN at same location
# %%
dst_new = xu.BarycentricInterpolator(source=src, target=like).regrid(src)
assert not np.any(np.isnan(dst_new) != np.isnan(dst))
# AssertionError, so NaN at different location. Turns out is one single strip of
# cells at the edge of the inactive area 
# %%
# Print number of NaN rows in the regridded arrays
print(len(np.unique(np.nonzero((np.isnan(dst)).values)[1])))
# 248
print(len(np.unique(np.nonzero((np.isnan(dst_new)).values)[1])))
# 250

# So new interpolator has 2 more NaN rows than the new one
# %%
## TEST CASE: DOWNSCALING GRID WITH LINE OF INCREASING VALUES IN CENTER
src = imod.util.empty_3d(dcell_coarse, xmin, xmax, dcell_coarse, ymin, ymax, layer)
like = imod.util.empty_2d(dcell_fine, xmin, xmax, dcell_fine, ymin, ymax)
src[:, 200, :] = np.arange(src.shape[1]) + 1

# %%
dst = imod.prepare.Regridder(method="multilinear", use_relative_weights=True).regrid(src, like)
dst_new = xu.OverlapRegridder(source=src, target=like, method="mean").regrid(src)
assert not np.any(np.isnan(dst_new) != np.isnan(dst))
# %%
dst_new = xu.BarycentricInterpolator(source=src, target=like).regrid(src)
# Print number of non-NaN rows in the regridded arrays
print(np.unique(np.nonzero((~np.isnan(dst)).values)[1]))
# array([500, 501, 502])
print(np.unique(np.nonzero((~np.isnan(dst_new)).values)[1]))
# array([499, 500, 501, 502, 503])

# So new interpolator has 2 more NaN rows than the new one

@Huite
Copy link
Contributor

Huite commented Feb 27, 2025

The barycentric interpolator considers points "on edge" to be inside, because of unstructured topologies and cell bounds that aren't axis aligned. Discounting points "on edge" results in holes appearing in the result. The old regridder doesn't have to worry about this, and can purposely bias a left or right edge.

This is the same issue as with the well placement.

JoerivanEngelen added a commit that referenced this issue Feb 28, 2025
Fixes #1372

# Description
See linked issue for all my testing, I think xugrid's regridders are a
better replacement of ``imod.prepare.Regridder``! Time to report the
public! There is one edge case which I documented in the changelog.

- Extends tests to also verify the old Regridder works the same as
xugrids regridder. I didn't bother with a big refactor to add this as
separate tests, as I foresee that we will remove these unittests
altogether when the iMOD Python's Legacy Regridder class is removed.
- Extend documentation with additional pointers to xugrid's regridders.
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in iMOD Suite Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants