Fix crop_with_convex_hull crash when margin_thickness is zero

[RajdeepKushwaha5]

Describe your changes

create_convex_hull_mask() returns a tuple (da_interior_mask, chull_lat_lons), but the margin_thickness == 0.0 branch in crop_with_convex_hull() (line 339) assigned the full tuple to da_mask without unpacking it. When _mask_with_common_dim() subsequently tried to access da_mask.dims, it raised an AttributeError because tuples have no .dims attribute.

Changes:

1. cropping.py line 339 — unpack the tuple: da_mask, _ = create_convex_hull_mask(...)
2. cropping.py line 31 — corrected the return type annotation on create_convex_hull_mask() from -> xr.DataArray to -> Tuple[xr.DataArray, xr.Dataset] to match its actual return value.
3. test_convex_hull_cropping.py — added test_crop_with_zero_margin_thickness regression test covering both the default path (include_interior_points=True) and the return_mask=True path.
4. CHANGELOG.md — added entry under [unreleased] > Fixes.

No new dependencies are required.

Issue Link

Closes #99

Type of change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

• My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
• I have performed a self-review of my code
• For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
• I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
• I have updated the documentation to cover introduced code changes
• I have added tests that prove my fix is effective or that my feature works
• I have given the PR a name that clearly describes the change, written in imperative form (context).
• I have requested a reviewer and an assignee (assignee is responsible for merging)

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

• the code is readable
• the code is well tested
• the code is documented (including return types and parameters)
• the code is easy to maintain

Author checklist after completed review

• I have added a line to the CHANGELOG describing this change, in a section
  reflecting type of change (add section where missing):
  • added: when you have added new functionality
  • changed: when default behaviour of the code has been changed
  • fixes: when your contribution fixes a bug

Checklist for assignee

• PR is up to date with the base branch
• the tests pass
• author has added an entry to the changelog (and designated the change as added, changed or fixed)

[Copilot]

Pull request overview

Fixes a crash in crop_with_convex_hull() when margin_thickness == 0.0 by correctly unpacking the (mask, hull_points) tuple returned by create_convex_hull_mask(), and adds a regression test plus a changelog entry.

Changes:

• Unpack create_convex_hull_mask() return value in the zero-margin branch to avoid passing a tuple into _mask_with_common_dim().
• Correct create_convex_hull_mask() return type annotation to match its actual tuple return value.
• Add regression test coverage for margin_thickness=0.0 (including return_mask=True) and document the fix in CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
mllam_data_prep/ops/cropping.py	Fixes the zero-margin crash path and updates the return type annotation.
tests/test_convex_hull_cropping.py	Adds a regression test for the zero-margin convex hull cropping behavior.
CHANGELOG.md	Records the bugfix under [unreleased] > Fixes.

Comments suppressed due to low confidence (1)

mllam_data_prep/ops/cropping.py:51

• create_convex_hull_mask builds da_interior_mask for points in ds (it runs contains_lonlat on da_lon/da_lat from ds), but the docstring Return description says the mask indicates points in ds_reference. Updating the docstring (and the da/da_ref wording) would prevent confusion for callers and future maintenance.

    """
    Create a grid-point mask for lat/lon coordinates in `da` indicating which
    points are interior to the convex hull of the lat/lon coordinates of
    `da_ref`.

    Parameters
    ----------
    ds : xarray.Dataset
        The dataset for which to create the mask.
    ds_reference : xarray.Dataset
        The reference dataset from which to create the convex hull of the coordinates.

    Returns
    -------
    xarray.DataArray
        A boolean mask indicating which points in `ds_reference` are interior
        to the convex hull.
    xarray.Dataset
        A dataset containing lat lon coordinates for points in `ds` making up
        the convex hull.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The exception message refers to exclude_interior=False, but the function parameter is include_interior_points. This makes the error hard to interpret; consider rewording the message to match the actual parameter name and condition that triggers the exception.

Suggested change

	"With no margin and exclude_interior=False, all points would be excluded."
	"With no margin and include_interior_points=False, all points would be excluded."

[Copilot]

The changelog entry says the crash happened "due to unpack of tuple"; it was actually caused by not unpacking the tuple returned by create_convex_hull_mask. Rewording this line would make the release note accurate.

Suggested change

	- fix `crop_with_convex_hull` crash when `margin_thickness == 0.0` due to unpack of tuple returned by `create_convex_hull_mask` [\#99](https://github.com/mllam/mllam-data-prep/issues/99) @RajdeepKushwaha5
	- fix `crop_with_convex_hull` crash when `margin_thickness == 0.0` caused by not unpacking the tuple returned by `create_convex_hull_mask` [\#99](https://github.com/mllam/mllam-data-prep/issues/99) @RajdeepKushwaha5

[Copilot]

This test creates a TemporaryDirectory() but never explicitly closes it. Using a context manager (or pytest’s tmp_path fixture) would ensure cleanup even if the test fails and avoids leaking temp directories on some platforms.

Suggested change

	tmpdir = tempfile.TemporaryDirectory()
	domain_size = 500 * 1.0e3
	N = 50
	config_lam = testdata.create_input_datasets_and_config(
	identifier="lam",
	data_categories=["state"],
	tmpdir=tmpdir,
	xlim=[-domain_size / 2.0, domain_size / 2.0],
	ylim=[-domain_size / 2.0, domain_size / 2.0],
	nx=N,
	ny=N,
	add_latlon=True,
	)
	config_global = testdata.create_input_datasets_and_config(
	identifier="global",
	data_categories=["state"],
	tmpdir=tmpdir,
	xlim=[-domain_size, domain_size],
	ylim=[-domain_size, domain_size],
	nx=N // 2,
	ny=N // 2,
	add_latlon=True,
	)
	ds_lam = mdp.create_dataset(config=config_lam)
	ds_global = mdp.create_dataset(config=config_global)
	# this previously crashed with AttributeError: 'tuple' object has no
	# attribute 'dims' because create_convex_hull_mask returns a tuple
	ds_cropped = cropping.crop_with_convex_hull(
	ds=ds_global,
	ds_reference=ds_lam,
	margin_thickness=0.0,
	include_interior_points=True,
	)
	assert isinstance(ds_cropped, xr.Dataset)
	# with zero margin and interior points included, the result should have
	# fewer points than the full global domain
	assert ds_cropped.grid_index.size < ds_global.grid_index.size
	assert ds_cropped.grid_index.size > 0
	# also test the return_mask path
	ds_cropped_2, da_mask = cropping.crop_with_convex_hull(
	ds=ds_global,
	ds_reference=ds_lam,
	margin_thickness=0.0,
	include_interior_points=True,
	return_mask=True,
	)
	assert isinstance(da_mask, xr.DataArray)
	assert da_mask.dtype == bool
	with tempfile.TemporaryDirectory() as tmpdir:
	domain_size = 500 * 1.0e3
	N = 50
	config_lam = testdata.create_input_datasets_and_config(
	identifier="lam",
	data_categories=["state"],
	tmpdir=tmpdir,
	xlim=[-domain_size / 2.0, domain_size / 2.0],
	ylim=[-domain_size / 2.0, domain_size / 2.0],
	nx=N,
	ny=N,
	add_latlon=True,
	)
	config_global = testdata.create_input_datasets_and_config(
	identifier="global",
	data_categories=["state"],
	tmpdir=tmpdir,
	xlim=[-domain_size, domain_size],
	ylim=[-domain_size, domain_size],
	nx=N // 2,
	ny=N // 2,
	add_latlon=True,
	add_latlon=True,
	)
	ds_lam = mdp.create_dataset(config=config_lam)
	ds_global = mdp.create_dataset(config=config_global)
	# this previously crashed with AttributeError: 'tuple' object has no
	# attribute 'dims' because create_convex_hull_mask returns a tuple
	ds_cropped = cropping.crop_with_convex_hull(
	ds=ds_global,
	ds_reference=ds_lam,
	margin_thickness=0.0,
	include_interior_points=True,
	)
	assert isinstance(ds_cropped, xr.Dataset)
	# with zero margin and interior points included, the result should have
	# fewer points than the full global domain
	assert ds_cropped.grid_index.size < ds_global.grid_index.size
	assert ds_cropped.grid_index.size > 0
	# also test the return_mask path
	ds_cropped_2, da_mask = cropping.crop_with_convex_hull(
	ds=ds_global,
	ds_reference=ds_lam,
	margin_thickness=0.0,
	include_interior_points=True,
	return_mask=True,
	)
	assert isinstance(da_mask, xr.DataArray)
	assert da_mask.dtype == bool

[Copilot]

ds_cropped_2 is assigned but never used; with the repo’s flake8 configuration (select = ... F ...), this will raise F841 (local variable assigned but never used) and fail linting. If you only need the mask, consider unpacking into _ instead.

Suggested change

	ds_cropped_2, da_mask = cropping.crop_with_convex_hull(
	_, da_mask = cropping.crop_with_convex_hull(