Skip to content

Additional Method to Drop NaN - #993

Open
Baran-Oz wants to merge 2 commits into
mainfrom
drop_NaN
Open

Additional Method to Drop NaN#993
Baran-Oz wants to merge 2 commits into
mainfrom
drop_NaN

Conversation

@Baran-Oz

Copy link
Copy Markdown
Collaborator
  • the existing dropna() method is modfied as per discussion in pandas-like dropna method #992
  • the current implementation used to remove all the time instances with any of the items being NaN
  • the new implementation is inpired by pandas where users get to decide weather they want to remove the time instances with all or any of the items being NaN.
  • Version is updated

@Baran-Oz
Baran-Oz requested a review from ecomodeller as a code owner June 30, 2026 12:59

@ecomodeller ecomodeller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 how and raising ValueError is 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/performance makes 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.dev03.3.0.dev1 — version bumps are a release action, not a per-PR action. The .dev0 suffix marks "unreleased code" (see CLAUDE.md); it isn't a counter. If every PR bumps it, every PR conflicts in pyproject.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.

@ecomodeller

Copy link
Copy Markdown
Member

@Baran-Oz any update?

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.

2 participants