Conversation
ecomodeller
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @Baran-Oz — the core idea is right and the implementation is a genuine improvement over what was there. Detailed notes below, with a bit of the reasoning behind each so they're useful beyond this PR.
What works well
- The default preserves existing behaviour. The old code intersected "not-all-NaN" indices across items, i.e. it dropped a step if any item was missing.
how="any"as the default keeps that, so existing scripts are unaffected. That's the right instinct — a new option should never silently change what current users get. - You also spotted that the old docstring ("Remove time steps where all items are NaN") contradicted the code, and fixed it.
- The new implementation is simpler than the old one. One boolean
(n_items, n_time)matrix plus.any()/.all()replaces an intersect-in-a-loop, and it removes the# this seems overly complicated...comment honestly rather than leaving it hanging. - Validating
howand raisingValueErroris better than silently treating anything that isn't"all"as"any".
Blocking
1. No tests
A new public API parameter needs tests. tests/test_dataset.py::test_dropna already builds the ideal fixture — d1 NaN from step 9, d2 from step 8 — so this is short:
ds.dropna() # 8 timesteps — guards the unchanged default
ds.dropna(how="all") # 9 timesteps
with pytest.raises(ValueError, match="how"):
ds.dropna(how="foo")The how="all" case is the whole point of the PR, and nothing currently proves it works.
2. Unrelated changes
Two files in the diff are unrelated to dropping NaNs:
.vscode/settings.json— removing--ignore=tests/performancemakes VS Code run the performance suite for every contributor who uses that config. That's a local editor preference leaking into a feature PR.pyproject.toml,3.3.0.dev0→3.3.0.dev1— version bumps are a release action, not a per-PR action. The.dev0suffix marks "unreleased code" (seeCLAUDE.md); it isn't a counter. If every PR bumps it, every PR conflicts inpyproject.toml.
The general principle: a PR should be reviewable as one idea. Anything a reviewer has to ask "why is this here?" about costs more than it saves.
Should fix
3. how should be Literal["any", "all"] and keyword-only
The repo already does this elsewhere — Dataset.interp_like(method: Literal["nearest", "inverse_distance"]) and Dataset.concat(keep: Literal["last", "first"]). And pandas' own DataFrame.dropna — the API you're modelling — is keyword-only with a Literal["any", "all"] alias. Keyword-only is what lets pandas add parameters like thresh and subset later without breaking positional callers; the same argument applies here.
def dropna(self, *, how: Literal["any", "all"] = "any") -> Dataset:Keep the runtime ValueError as well — mypy doesn't run in a user's notebook.
4. Two small cleanups in the body
n_time = self[0].to_numpy().shape[0]This materialises an array just to read a length. self.n_timesteps gives the same number for free.
keep: Any = list(np.where(~drop)[0])IndexType already includes np.ndarray, so keep = np.where(~drop)[0] types cleanly with no Any and no list round-trip. The : Any was a workaround for the old code path — worth noticing when an inherited workaround is no longer needed.
Minor
5. Keep the docstring paragraph about what "missing" means
Spelling out that an item counts as missing only when its entire field is NaN — and that scattered NaNs like dry cells don't count — is the most valuable part of this diff. It's a real divergence from pandas, where how="any" drops a row on a single NaN, so users arriving from pandas will otherwise assume the pandas meaning. Consider adding an Examples section too; quartodoc renders it into the API docs.
6. DataArray.dropna
src/mikeio/dataset/_dataarray.py:461 still says "Remove time steps where values are NaN", which has the same inaccuracy you just fixed on Dataset. how doesn't apply to a single item, but the one-line docstring fix belongs here while the context is fresh.
This review was authored by Claude at the maintainer's request.
|
@Baran-Oz any update? |
alloranyof the items being NaN.