Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

      9 files  ±  0        9 suites  ±0   3h 10m 40s ⏱️ - 1m 40s
 18 126 tests + 24   16 909 ✅ + 24   1 217 💤 ±0  0 ❌ ±0 
162 208 runs  +216  150 092 ✅ +215  12 116 💤 +1  0 ❌ ±0 

Results for commit 09d4126. ± Comparison against base commit c60725b.

♻️ This comment has been updated with latest results.

@mmccarty would you have some time to review this?

map_blocks also accepts a dtype parameter, but it does not actually convert data to that type, which is quite puzzling. Therefore, I wonder whether we should add type conversion in map_blocks instead

The purpose of that dtype parameter is to let dask know the dtype of the output ndarray before executing the dask graph. I don't think we should overload it to also mean "cast the output to this dtype".

It would also be a good to test that the result dtype matches the input array's dtype when the parameter isn't specified in cumprod.

Thanks, @tonyyuyiding!
@jacobtomlinson - Nothing further from me.

@TomAugspurger I take your comment to be blocking on the implementation here, is that right?

I hadn't looked at the diff yet, just the description :)

So IIUC, the root cause here is a clash between a func (like np.cumprod) which takes a dtype param and map_blocks (slightly different) usage of dtype. Does that sound right @tonyyuyiding?

I'd be curious whether we could do func = partial(func, dtype=dtype).map_blocks(func, axis=axis, dtype=dtype)? In this case, we happen to know that e.g. np.cumprod(data, dtype=dtype) means the result type will be dtype (and cumprod takes care of the casting to ensure that's true), so the meaning of dtype is the same across the two functions.

So IIUC, the root cause here is a clash between a func (like np.cumprod) which takes a dtype param and map_blocks (slightly different) usage of dtype. Does that sound right?

Yes, that sounds right to me.

I'd be curious whether we could do func = partial(func, dtype=dtype).map_blocks(func, axis=axis, dtype=dtype)? In this case, we happen to know that e.g. np.cumprod(data, dtype=dtype) means the result type will be dtype (and cumprod takes care of the casting to ensure that's true), so the meaning of dtype is the same across the two functions.

Thanks for the suggestion! I agree that using partial is better, in case some other func does not do type conversion.

Great, thanks.

My only remaining concern is whether we can assume that func always takes a dtype= argument. The docs are not clear on this:

Cumulative function like np.cumsum or np.cumprod

We can maybe treat "like" as meaning "has the same function signature and call this good enough, assuming the tests pass.

My only remaining concern is whether we can assume that func always takes a dtype= argument.

I found that some func does not take dtype. Perhaps we can use inspect to check whether func accepts dtype, but I'm not sure whether it is the best practice.

Perhaps we can use inspect to check whether func accepts dtype,

Yep. That's a bit unfortunate, but I think that's our best option.

I didn't mention it earlier, but I'd like to leave as much up to NumPy as possible. It's not 100% clear to me that np.cumprod(x.astype(dtype)) is the same in all cases as np.cumprod(x, dtype=dtype), and I'd like to not have to worry about that.

If you're able to, it'd be nice to have a test that covers the case where func doesn't accept a dtype parameter.

I checked the places where cumreduction is used. It seems that the only case where func does not accept dtype is inside the push function, which is Dask-version of bottleneck.push. That push function is quite different from cumsum and cumprod, and it has its own tests in another file. I see those tests already failed when I used partial without checking whether func accepts dtype. Therefore, I guess we don't need to add additional tests here?