Add impls of ProgramNode for various math operations - #16107
Conversation
|
One or more of the following people are relevant to this code:
|
Coverage Report for CI Build 33116403395Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.5%) to 87.74%Details
Uncovered Changes
Coverage Regressions2610 previously-covered lines in 48 files lost coverage.
Coverage Stats
💛 - Coveralls |
b429b8d to
a437668
Compare
31adc4f to
768edb2
Compare
|
One or more of the following people are relevant to this code:
|
a590a50 to
2be78bc
Compare
73504ec to
d590df2
Compare
I've added a few more tests now, and in general I think coverage is decent. If there are any specific missing spots you see, please let me know and I'll add more. |
6b8b47b to
337dba8
Compare
This lead me to also realize that we should handle However, for integer types, we panic. We should instead do what NumPy does, and return 0 on I'll do a new PR for this. |
1b0409c to
f3d7dbc
Compare
mtreinish
left a comment
There was a problem hiding this comment.
Overall I think this is almost good to go. The only thing that stood out to me is the array operations in the reduction module are allocating a bit more than is necessary. I think we can reduce the amount of allocations by mutating some of the intermediate arrays inplace. I left comments on some spots that I saw but I'm not sure I caught them all.
I also left some inline nits on removing the static lifetime from all the functions that return string. It's not necessary as it's implied by returning an inline string literal and it doesn't really convey anything useful.
| let n = a.shape()[self.axis] as f64; | ||
| let mean = (a.sum_axis(Axis(self.axis)) / Complex::new(n, 0.0)) | ||
| .insert_axis(Axis(self.axis)); | ||
| let sq_mod = (a - &mean).mapv(|c| c.re * c.re + c.im * c.im); |
There was a problem hiding this comment.
I think we can probably do mapv_inplace here too and avoid another allocation since the subtraction should be allocating a new array already too. But it might require a bit large of a change than is easy inline.
There was a problem hiding this comment.
I applied your other mapv_inplace suggestions, but then ended up reverting because they were wrong for a trivial reason: stds/vars of a complex vector are real, so we can't put a float into a complex slot in-place.
However, you sent me down a mini rabbit hole that also addresses this comment: see aea04da, which is both faster (in some regims) and more memory efficient than the current implementation.
There was a problem hiding this comment.
| shape | axis | input | original ms / peak | lane ms | slice ms | dispatch ms / peak |
|---|---|---|---|---|---|---|
[1024,1024] |
0 | 8.0 MiB | 1.72 / 12.0 MiB | 1.69 | 0.58 | 0.58 / 0.02 MiB |
[1024,1024] |
1 | 8.0 MiB | 1.66 / 12.0 MiB | 1.26 | 1.94 | 1.26 / 0.02 MiB |
[100,100000] |
0 | 76.3 MiB | 19.18 / 115.2 MiB | 12.97 | 6.90 | 6.85 / 1.53 MiB |
[100000,100] |
1 | 76.3 MiB | 22.05 / 115.2 MiB | 9.21 | 33.02 | 8.68 / 1.53 MiB |
[64,128,512] |
1 | 32.0 MiB | 9.32 / 48.2 MiB | 7.57 | 5.41 | 4.95 / 0.50 MiB |
[4096,4096] |
0 | 128.0 MiB | 29.44 / 192.0 MiB | 95.37 | 8.47 | 8.25 / 0.06 MiB |
[1000000,4] |
0 | 30.5 MiB | 107.18 / 45.8 MiB | 50.01 | 101.35 | 49.75 / 0.00 MiB |
[4,1000000] |
1 | 30.5 MiB | 5.87 / 45.8 MiB | 5.15 | 76.61 | 5.13 / 0.00 MiB |
[1000000] |
0 | 7.6 MiB | 1.88 / 11.4 MiB | 1.27 | 54.03 | 1.27 / 0.00 MiB |
[2,2000000] |
0 | 30.5 MiB | 11.00 / 61.0 MiB | 11.16 | 8.26 | 6.35 / 30.5 MiB |
[2000000,2] |
1 | 30.5 MiB | 42.72 / 61.0 MiB | 13.65 | 12.72 | 12.32 / 30.5 MiB |
[1024,1024] strided view |
1 | 8.0 MiB | 5.62 / 12.0 MiB | 2.25 | 4.27 | 2.24 / 0.02 MiB |
[2048,2048] transposed |
0 | 32.0 MiB | 7.67 / 48.0 MiB | 5.22 | 15.11 | 5.22 / 0.03 MiB |
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This reverts commit 0dc1c02.
mtreinish
left a comment
There was a problem hiding this comment.
This LGTM now. Just one inline nit on the bare unreachable macros. It'd be good to add a reason that we think it's unreachable just for diagnostic when debugging an issue if it somehow got tripped and things panic.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This commit expands the test coverage of the reduction module to make sure all the code paths are exercised and validated. It was mostly dtype coverage for all the math nodes that was missing. The other piece was the optimizations added aea04da the accumulation path was not covered so a test was added and compared against the result from ndarray's var axis.
This PR closes #16031 by implementing ProgramNode for various math-y structs.
PR Stack
AI/LLM disclosure