Skip to content

Fix AttributeError in facebudget when front is omitted (fixes #1097)#1849

Open
gaoflow wants to merge 2 commits into
Deltares:masterfrom
gaoflow:fix/facebudget-omit-front-none-deref
Open

Fix AttributeError in facebudget when front is omitted (fixes #1097)#1849
gaoflow wants to merge 2 commits into
Deltares:masterfrom
gaoflow:fix/facebudget-omit-front-none-deref

Conversation

@gaoflow
Copy link
Copy Markdown
Contributor

@gaoflow gaoflow commented Jun 1, 2026

Summary

imod.evaluate.facebudget documents and enforces that at least one of
front, lower, right must be supplied (it raises ValueError if all three
are None). However two code paths dereferenced front unconditionally, so
omitting front crashed:

def facebudget(budgetzone, front=None, lower=None, right=None, netflow=True):
    ...
    if "time" in dims:
        for itime in range(front.coords["time"].size):   # front may be None
    ...
    else:                                                 # no zones defined
        dask_front = dask.array.full(front.shape, ...)    # front may be None
        dask_lower = dask.array.full(lower.shape, ...)    # lower may be None
        dask_right = dask.array.full(right.shape, ...)    # right may be None

So a perfectly valid call such as facebudget(budgetzone, lower=lower) raised:

AttributeError: 'NoneType' object has no attribute 'coords'   # transient case
AttributeError: 'NoneType' object has no attribute 'shape'    # no-zones case

This is exactly the null-dereference Coverity flagged in #1097
(REVERSE_INULL: front is dereferenced on all paths before the
if front is not None guard inside the loop). The other location named in
#1097 (package.py:292) no longer exists — that code was refactored into
imod/evaluate/budget.py, which is the surviving occurrence fixed here.

Fix

Pick the first supplied DataArray as a reference for the time axis and the
output shape. All three inputs are already validated to share budgetzone's
spatial shape, so any of them is a valid reference:

reference = next(da for da in (front, lower, right) if da is not None)
...
for itime in range(reference.coords["time"].size):
...
dask_front = dask.array.full(reference.shape, np.nan, chunks=chunks)
dask_lower = dask.array.full(reference.shape, np.nan, chunks=chunks)
dask_right = dask.array.full(reference.shape, np.nan, chunks=chunks)

Reproduction / verification

import numpy as np, xarray as xr, imod
da = xr.DataArray(np.zeros((3, 2, 1)),
                  {"layer": [1, 2, 3], "y": [0.5, 1.5], "x": [0.5]},
                  ("layer", "y", "x"))
lower = xr.full_like(da, 0.0); lower[:2] = 1.0
zone = xr.full_like(da, np.nan); zone[:2] = 1
imod.evaluate.facebudget(zone, lower=lower)   # before: AttributeError; after: works

Added test_omit_front and test_omit_front_transient to
imod/tests/test_evaluate/test_budget.py. The transient test exercises both
crash sites (the time loop and the no-zones branch) via a partial zone and an
all-inactive zone. Verified locally: both tests fail on master
(AttributeError) and pass with this change; all pre-existing budget tests
(9) still pass.

Fixes #1097.

facebudget requires only one of front/lower/right (it raises if all three
are None), but two code paths dereferenced front unconditionally:

* the transient loop bound `range(front.coords["time"].size)`, and
* the no-zones branch `dask.array.full(front.shape, ...)` (and the
  matching lower/right lines).

So calling e.g. `facebudget(zone, lower=lower)` raised
`AttributeError: 'NoneType' object has no attribute 'coords'` (or
'shape'). This is the null-dereference flagged by Coverity in Deltares#1097.

Pick the first supplied DataArray as a reference for the time axis and
the output shape (all three are validated to share budgetzone's shape).
Add regression tests covering the omitted-front case with and without a
time dimension, including the no-zones branch.
@JoerivanEngelen
Copy link
Copy Markdown
Contributor

JoerivanEngelen commented Jun 4, 2026

@gaoflow

Looks good to me, could you also add your fix to the changelog? Write about it with users in mind, who are not software developers.

@gaoflow
Copy link
Copy Markdown
Contributor Author

gaoflow commented Jun 4, 2026

Done — added a Fixed entry to the changelog under [Unreleased], phrased for users: it notes that facebudget previously errored when you left out front, even though only one of front/lower/right is required, and that omitting front now works as documented.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 4, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

None access in budget.py and package.py

2 participants