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

refactor: np.where(cond) -> np.asarray(cond).nonzero() #2238

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Jun 17, 2024

The docs for np.where() (https://numpy.org/doc/stable/reference/generated/numpy.where.html) suggest to prefer nonzero() over where() without x and y arguments. In the spirit of defensive programming I included np.asarray(cond) even where cond is already an array.

This PR also fixes a bug I introduced in the model splitter in #2124: while E711 (https://www.flake8rules.com/rules/E711.html) dictates comparisons to None should use identity rather than equality, this rule should not be applied to NumPy array selection conditions as it will change the semantics:

>>> a = np.array([None, None])
>>> a[a != None]
array([], dtype=object)
>>> a[a is not None]
array([[None, None]], dtype=object)

Unrelatedly, mark test_mt3d.py::test_mfnwt_keat_uzf() slow, it should not be included in smoke tests (this was causing the optional dependency CI tests to fail due to timeout). And clean up some unused imports in conftest.py.

The docs for `np.where()` (https://numpy.org/doc/stable/reference/generated/numpy.where.html) suggest to prefer `nonzero()` over `where()` without `x` and `y` arguments. In the spirit of defensive programming I included `np.asarray(cond)` even where `cond` is already an array.

This PR also fixes a bug I introduced in the model splitter in modflowpy#2124: while E711 (https://www.flake8rules.com/rules/E711.html) dictates comparisons to `None` should use identity rather than equality, this rule should not be applied to NumPy array selection conditions as it will change the semantics:

```shell
>>> a = np.array([None, None])
>>> a[a != None]
array([], dtype=object)
>>> a[a is not None]
array([[None, None]], dtype=object)
```

Unrelatedly, mark `test_mt3d.py::test_mfnwt_keat_uzf()` slow, it should not be included in smoke tests.
@wpbonelli wpbonelli requested a review from mwtoews June 17, 2024 12:52
@wpbonelli wpbonelli marked this pull request as ready for review June 17, 2024 14:12
Copy link
Contributor

@mwtoews mwtoews left a comment

Choose a reason for hiding this comment

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

While np.asarray() isn't always necessary for objects that are already arrays, it shouldn't have any performance impacts (arrays are copied only if necessary, otherwise it just passes the object pointer, thus fast). All other changes look fine to me.

@wpbonelli wpbonelli merged commit 59040d0 into modflowpy:develop Jun 17, 2024
24 checks passed
@wpbonelli wpbonelli deleted the numpy-where branch June 17, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants