Conversation
keewis
left a comment
keewis
left a comment
There was a problem hiding this comment.
thanks for taking this on.
This looks good to me in general, but there might be a few things that we could do differently (especially the numpy.fix support I believe we can deprecate or drop)
| @pytest.mark.filterwarnings("ignore:numpy.fix is deprecated.") | ||
| def test_fix() -> None: | ||
| val = 3.0 | ||
| val_fixed = np.fix(val) |
There was a problem hiding this comment.
we could also just remove (or deprecate?) support for fix, there's a clear alternative in np.trunc that does the same and is more efficient
There was a problem hiding this comment.
Yeah sorry I meant to put notes on each of these. I looked at the PR where this was added and it was specifically resolving an infinite recursion that was possible within apply_ufunc for certain kwargs. I just checked and you don't get the same recursion issue with np.trunc. So I think the alternative is to delete the test.
| try: | ||
| result_type = dtypes.result_type(*original_coords) | ||
| except TypeError: | ||
| except (TypeError, ValueError): | ||
| pass |
There was a problem hiding this comment.
It looks like this was only supposed to cast object dtype arrays to numpy string, in which case this is fine.
If we actually do want to cast the pandas string dtype to a numpy string dtype, however, we may need to change that (@dcherian, do you remember what our policy on pandas dtypes was?)
There was a problem hiding this comment.
Right yeah there might be something else that needs doing to support these more completely. For instance you can't write them to netcdf or zarr, but I tested with older versions of pandas/numpy and it looks like this test has always hit this except it's just the type changed from TypeError to ValueError
There was a problem hiding this comment.
If we actually do want to cast the pandas string dtype to a numpy string dtype,
I think at the moment we do; and then relax it intentionally. However, we seem to have done pretty badly at that haha. Things seem to keep breaking.
There was a problem hiding this comment.
Ok so something to keep working towards, but for the purposes of getting CI green again, maybe no worse than it was before is fine for now?
| with pytest.raises(ValueError): | ||
| _get_func_args(np.power, []) |
There was a problem hiding this comment.
do you know what about numpy.power changed? I can't find a PR / changelog entry other than one that changes its behavior on a old CPU architecture (AIX)
There was a problem hiding this comment.
I didn't find any information specifically on np.power but from reading the comments on the original PR it seemed like sometimes function parameters weren't discoverable by inspect.signature my understanding is that this had to do with how ufuncs are constructed in numpy or possibly it has to do with C based functions. I just figured numpy might have got a better system for passing parameter information around.
There was a problem hiding this comment.
I just figured numpy might have got a better system for passing parameter information around
yeah i think ufuncs have better python-accessible signatures now
|
@keewis it looks like these are affecting CI on main now. Is there anything you want me to change? |
|
sorry, I've been a bit absent. Yes, let's merge and continue working on the pandas dtypes afterwards. |
3 participants
Nightly tests running in https://github.com/pydata/xarray/actions/runs/22463416977/job/65063416027?pr=11204