Skip to content

Comments

Ensure the slice is an array#114

Merged
efiring merged 3 commits intoTEOS-10:mainfrom
ocefpaf:fix_85
Nov 29, 2022
Merged

Ensure the slice is an array#114
efiring merged 3 commits intoTEOS-10:mainfrom
ocefpaf:fix_85

Conversation

@ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Nov 29, 2022

@DocOtak I'm not sure if we can do something fancier here than just casting to an array. Do you have any suggestions?

Closes #95

@DocOtak
Copy link
Contributor

DocOtak commented Nov 29, 2022

It kinda sounds like numpy is unhappy with whatever the 'indexer' function is returning?

@ocefpaf
Copy link
Member Author

ocefpaf commented Nov 29, 2022

It kinda sounds like numpy is unhappy with whatever the 'indexer' function is returning?

In the issue @rcaneill found out that, when using xarray datarrays, the igood indexer will be a datarray too and numpy doesn't like that. I'm casting it to an array but updating to numpy 1.23 also fixes that, which makes me think that they are OK with DataArray in newer versions of numpy. We can bump our min numpy or add this workaround.

order = 'C' # e.g., xarray DataArray doesn't have flags
for ind in indexer(SA.shape, axis, order=order):
igood = goodmask[ind]
igood = np.array(goodmask[ind])
Copy link
Member

Choose a reason for hiding this comment

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

I think that using np.asarray would be more efficient, avoiding the copy which is unnecessary when the original function arguments are numpy arrays. I would also add a comment that this is needed to support xarray inputs for numpy < 1.23.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

The reason I suggested asarray rather than asanyarray is that the point is to end up with a plain ndarray as the boolean indexing object. It's unlikely to make any difference in practice, since masked arrays will never end up here and DataArrays (and Pandas structures, for that matter) are not ndarray subclasses, at least in recent versions; but I think that asarray still has the advantage of clarifying the intent.

@ocefpaf
Copy link
Member Author

ocefpaf commented Nov 29, 2022

The reason I suggested asarray rather than asanyarray is that the point is to end up with a plain ndarray as the boolean indexing object.

VScode completed to asany and I did not even noticed! I literally copied-and-pasted from your comment. Fixing it in a moment...

@efiring efiring merged commit d7684d9 into TEOS-10:main Nov 29, 2022
@ocefpaf ocefpaf deleted the fix_85 branch November 29, 2022 21:16
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.

Error when using xarray.DataArray and gsw.geo_strf_dyn_height with numpy < 1.23

3 participants