cubed icon indicating copy to clipboard operation
cubed copied to clipboard

Implementation of `std` and `var`.

Open alxmrs opened this issue 1 year ago • 4 comments

Fixes #29. Here, I use existing cubed operations to implement var and std. Please let me know if I should reimplement the primitives as pure reductions.

alxmrs avatar Nov 21 '23 03:11 alxmrs

Thank you for submitting a PR for this @alxmrs!

Please let me know if I should reimplement the primitives as pure reductions.

That's how I envisaged doing this (similar to Dask). The way you have it at the moment has two separate reductions which will be less efficient than one (and reductions in Cubed are quite inefficient at the moment, something we want to remedy in e.g. #284). Also I'm not sure if there are any differences in numerical stability between the two approaches.

tomwhite avatar Nov 21 '23 12:11 tomwhite

It would be good to get std/var into Cubed. I found an implementation that we could try here (see https://github.com/cubed-dev/cubed/issues/29#issuecomment-2368108624) - would you be interested in having a go at that @alxmrs?

I wonder whether we should merge this in the meantime though?

tomwhite avatar Sep 23 '24 12:09 tomwhite

Hey Tom. I don’t want to block you in the implementation of fundamental routines. How about I try to get this merged in the next three days/EOW, and if I can’t find the time, we close the PR? I appreciate how you’re trying to incorporate my patches to cubed; thanks, I feel included.

On Mon, Sep 23, 2024 at 9:49 PM Tom White @.***> wrote:

@.**** commented on this pull request.

In cubed/array_api/statistical_functions.py https://github.com/cubed-dev/cubed/pull/327#discussion_r1771339956:

@@ -152,3 +153,12 @@ def sum(x, /, *, axis=None, dtype=None, keepdims=False): keepdims=keepdims, extra_func_kwargs=extra_func_kwargs, )

+def var(x, /, *, axis=None, keepdims=False):

This doesn't match the signature in the specification https://data-apis.org/array-api/latest/API_specification/statistical_functions.html since it's missing a correction parameter. Similarly for std.

In cubed/tests/test_array_api.py https://github.com/cubed-dev/cubed/pull/327#discussion_r1771342079:

@@ -563,6 +563,74 @@ def test_sum_axis_0(spec, executor): assert_array_equal(b.compute(executor=executor), np.array([12, 15, 18]))

+def test_var(spec, executor):

Can you parameterize all these tests for var for different axes, and for keepdims? There are other tests in the test suite that do this.

— Reply to this email directly, view it on GitHub https://github.com/cubed-dev/cubed/pull/327#pullrequestreview-2322086316, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARXAB3V2ICY2Q2ROCXYXATZYAE5ZAVCNFSM6AAAAABOV77TMWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRSGA4DMMZRGY . You are receiving this because you were mentioned.Message ID: @.***>

alxmrs avatar Sep 24 '24 01:09 alxmrs

Thanks Alex - that sounds like a good plan!

(Sorry I didn't suggest how to move forward on this before - you weren't blocking me!)

tomwhite avatar Sep 24 '24 08:09 tomwhite