is_nonstr_iter: 0-D array is now recognized as non-iterable #4004
Conversation
seisman
added
bug
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the is_nonstr_iter function where 0-dimensional NumPy arrays were incorrectly identified as iterable when they should be treated as non-iterable scalar values.
- Updated the
is_nonstr_iterfunction to properly handle 0-D arrays by checking forndim == 0 - Enhanced function documentation to clarify that 0-D arrays are excluded
- Added docstring example demonstrating the correct behavior for 0-D arrays
| return ( | ||
| isinstance(value, Iterable) | ||
| and not isinstance(value, str) | ||
| and not (hasattr(value, "ndim") and value.ndim == 0) |
There was a problem hiding this comment.
[nitpick] The condition hasattr(value, "ndim") and value.ndim == 0 could be made more explicit by importing numpy and using isinstance(value, np.ndarray) instead of hasattr(value, "ndim"). This would be more specific and avoid potential false positives from other objects that might have an ndim attribute.
seisman
changed the title
seisman
added
final review call
pygmt/helpers/utils.py
Outdated
| return ( | ||
| isinstance(value, Iterable) | ||
| and not isinstance(value, str) | ||
| and not (isinstance(value, np.ndarray) and value.ndim == 0) |
There was a problem hiding this comment.
Should we just check for the ndim attribute, since it part of the Python array API standard? Xref https://data-apis.org/array-api/2024.12/API_specification/generated/array_api.array.ndim.html#ndim
| and not (isinstance(value, np.ndarray) and value.ndim == 0) | |
| and not (hasattr(value, "ndim") and value.ndim == 0) |
There was a problem hiding this comment.
Oh, I see you did that first, and then copilot suggested otherwise 😅
There was a problem hiding this comment.
This is exactly what I wrote in the intial version, then copilot made the suggestion at #4004 (comment).
There was a problem hiding this comment.
I think ok to use hasattr to avoid an import, and also allow for duck typing.
pygmt/helpers/utils.py
Outdated
| from pathlib import Path | ||
| from typing import Any, Literal | ||
|
|
||
| import numpy as np |
There was a problem hiding this comment.
| import numpy as np |
seisman
removed
the
final review call
Fixes #4001.