Remove `MaxAndArgmax` `Op` #874

aerubanov · 2022-03-23T13:32:21Z

Closes #765

brandonwillard

Looks like this is making good progress; much appreciated!

aesara/tensor/math.py

aesara/tensor/opt_uncanonicalize.py

brandonwillard · 2022-05-01T23:23:35Z

tests/link/test_numba.py

-def test_MaxAndArgmax(x, axes, exc):
-    g = aem.MaxAndArgmax(axes)(x)
-
-    if isinstance(g, list):
-        g_fg = FunctionGraph(outputs=g)
-    else:
-        g_fg = FunctionGraph(outputs=[g])
-
-    cm = contextlib.suppress() if exc is None else pytest.warns(exc)
-    with cm:
-        compare_numba_and_py(
-            g_fg,
-            [
-                i.tag.test_value
-                for i in g_fg.inputs
-                if not isinstance(i, (SharedVariable, Constant))
-            ],
-        )


We can add a new pytest.mark.parametrize and parameter that cycles through the two now independent Ops and runs the same tests.

brandonwillard · 2022-05-01T23:25:13Z

tests/tensor/nnet/test_basic.py

@@ -906,71 +895,6 @@ def validate_grad_graph(func):
            assert softmax_grad_legacy not in ops


-def test_argmax_pushdown():


It should be possible to refactor these so that they work with each Op separately.

brandonwillard · 2022-05-01T23:26:24Z

tests/tensor/test_math.py

@@ -745,400 +733,9 @@ def test_isnan():
        f([[0, 1, 2]])


-class TestMaxAndArgmax:


These tests need to be converted to work with the two Ops , as well.

brandonwillard · 2022-05-01T23:27:11Z

tests/tensor/test_opt_uncanonicalize.py

@@ -24,28 +22,6 @@
 from tests.link.test_link import make_function


-class TestMaxAndArgmax:


tests/tensor/test_math_opt.py

aerubanov · 2022-05-02T14:03:23Z

@brandonwillard, thanks for the review! I will work on the requested changes.

aerubanov · 2022-05-05T13:59:24Z

Hi @brandonwillard! I have the test that fails because max() here https://github.com/aerubanov/aesara/blob/b310d4dc10c959e5b7917159dea8397aceaba56f/aesara/tensor/math.py#L561 does not work correctly when the input tensor has a value that is the maximum of the uint64 type. I don't understand why this happens and what needs to be changed to fix it. Could you take a look when you have a chance?

brandonwillard

I just made some small fixes and rebased onto main. The errors I'm seeing now are due to the unfinished Numba implementations. ~~Is that older, more cryptic error still present?~~ (Nevermind, I see it in tests.tensor.test_math.TestMinMax.test_uint. I'll look into it.)

Since these commits need to be restructured/squashed anyway, it would be good to reorganize them so that the Max and ArgMax updates and their corresponding tests are added first (e.g. in a single commit or one for each Op along with their JAX and Numba implementations), then the removal/replacement of MaxAndArgmax in another commit.

aerubanov · 2022-05-31T14:02:18Z

@brandonwillard Thanks for the review! I'll be working on those changes.

codecov · 2022-06-10T16:07:54Z

Codecov Report

Merging #874 (af1f40b) into main (be222f0) will decrease coverage by 0.04%.
The diff coverage is 90.62%.

❗ Current head af1f40b differs from pull request most recent head 114bf01. Consider uploading reports for the commit 114bf01 to get more accurate results

@@            Coverage Diff             @@
##             main     #874      +/-   ##
==========================================
- Coverage   79.25%   79.20%   -0.05%     
==========================================
  Files         152      152              
  Lines       47882    47882              
  Branches    10909    10906       -3     
==========================================
- Hits        37949    37926      -23     
- Misses       7436     7454      +18     
- Partials     2497     2502       +5

Impacted Files	Coverage Δ
aesara/ifelse.py	`49.85% <ø> (+0.14%)`	⬆️
aesara/tensor/math.py	`90.06% <88.23%> (+0.26%)`	⬆️
aesara/link/jax/dispatch.py	`81.34% <100.00%> (-0.43%)`	⬇️
aesara/link/numba/dispatch/elemwise.py	`93.00% <100.00%> (-4.13%)`	⬇️
aesara/tensor/nnet/basic.py	`80.34% <100.00%> (+0.06%)`	⬆️
aesara/tensor/opt_uncanonicalize.py	`96.42% <100.00%> (+0.39%)`	⬆️
aesara/link/numba/dispatch/basic.py	`87.25% <0.00%> (-4.81%)`	⬇️
aesara/sparse/type.py	`72.11% <0.00%> (-2.66%)`	⬇️
aesara/link/numba/dispatch/tensor_basic.py	`97.95% <0.00%> (-2.05%)`	⬇️
... and 37 more

aerubanov · 2022-06-10T17:48:53Z

Hi @brandonwillard ! I seem to be able to fix the errors that occurred after removing MaxAndArgmax. Now I'm going to edit the commit history. But I would also like to ask you to review my changes and pay attention to the following points:

I implemented Max as COp using MaxAndArgmax code, because it allows me to fix tests.tensor.test_math.TestMinMax.test_uint (see previous comments). But may be it is not the best option;
I keep max_and_argmax() because it is part of API - now it create Max and Argmax nodes. Do we want to keep this function or I should make it deprecated?

brandonwillard

Looks like an extra file got added: aesara/tensor/\.

brandonwillard · 2022-07-16T23:24:15Z

tests/tensor/test_math_opt.py

+            # at_max,
+            # at_min,


Why have the max and min functions been removed from TestLocalReduce?

@brandonwillard I moved the MaxAndArgmax functionality to Max and now Max is implemented as a COp, so the tests for CAReduce will not work with Max.

brandonwillard · 2022-07-16T23:27:48Z

aesara/tensor/math.py

+    def R_op(self, inputs, eval_points):
+        raise ValueError("Argmax is not a differentiable operation")
+


Suggested change

def R_op(self, inputs, eval_points):

raise ValueError("Argmax is not a differentiable operation")

There's already a grad implementation—albeit effectively non-functional—so I don't know if it helps to have an R_op like this. Also, I don't think a ValueError would be the best here.

If a gradient is undefined, I believe we should return an aesara.gradient.grad_undefined-generated NullType. Same with unimplemented gradients: we should use aesara.gradient.grad_not_implemented.

…ly.github.com> modify Max Co-authored-by: Brandon T. Willard <[email protected]>

Co-authored-by: Brandon T. Willard <[email protected]>

aerubanov · 2022-07-19T09:10:28Z

@brandonwillard I removed Argmax`s R_op and replace Value Error on aesara.gradient.grad_undefined.

aerubanov · 2022-07-27T11:12:07Z

@brandonwillard, just soft reminder)

brandonwillard

Looks like one of the commit messages got scrambled.

brandonwillard · 2022-07-28T23:45:26Z

aesara/tensor/math.py

-class Max(NonZeroCAReduce):
-    nfunc_spec = ("max", 1, 1)
+class Max(COp):


Why aren't we using the NonZeroCAReduce base class for this?

@brandonwillard Thank you for review. To fix this #874 (comment) problem with Max I coped the code from MaxAndArgmax and made it COp. Do you think that it can add some problems?

It could interfere with existing code that assumes Max is a subclass of NonZeroCAReduce (e.g. rewrites, our JAX and/or Numba implementations, etc.)

Also, we generally want to make use of existing code. In this case, subclassing NonZeroCAReduce could make adding a C implementation much easier that it otherwise would be.

brandonwillard · 2022-07-28T23:47:56Z

tests/tensor/test_math.py

-        MaxAndArgmax.debug = 0
+        Argmax.debug = 0
+        Max.debug = 0


What do these do?

Hm, it looks like a piece of old code. I think I should to remove it.

aerubanov marked this pull request as draft March 23, 2022 13:32

brandonwillard added the refactor This issue involves refactoring label Apr 14, 2022

aerubanov force-pushed the maxandargmax_refactoring branch from c8b52f1 to 402be24 Compare April 25, 2022 15:06

brandonwillard changed the title ~~Remove MaxAndArgmax Op~~ Remove MaxAndArgmax Op Apr 28, 2022

brandonwillard requested changes May 1, 2022

View reviewed changes

brandonwillard force-pushed the maxandargmax_refactoring branch 2 times, most recently from d73f7e8 to 194bbbe Compare May 27, 2022 22:55

brandonwillard reviewed May 27, 2022

View reviewed changes

aerubanov force-pushed the maxandargmax_refactoring branch 2 times, most recently from 5a24cad to 91e3c4b Compare July 15, 2022 15:25

aerubanov marked this pull request as ready for review July 15, 2022 16:37

aerubanov requested a review from brandonwillard July 15, 2022 16:37

brandonwillard requested changes Jul 16, 2022

View reviewed changes

Co-authored-by: Brandon T. Willard <[email protected]…

ab2b87e

…ly.github.com> modify Max Co-authored-by: Brandon T. Willard <[email protected]>

aerubanov force-pushed the maxandargmax_refactoring branch from 2a134f5 to 0b0d854 Compare July 18, 2022 15:38

modify Argmax

ccf42be

Co-authored-by: Brandon T. Willard <[email protected]>

aerubanov force-pushed the maxandargmax_refactoring branch 2 times, most recently from f4e62f0 to 0d3c415 Compare July 18, 2022 18:59

remove MaxAndArgmax

114bf01

Co-authored-by: Brandon T. Willard <[email protected]>

aerubanov force-pushed the maxandargmax_refactoring branch from 0d3c415 to 114bf01 Compare July 18, 2022 20:38

aerubanov requested a review from brandonwillard July 19, 2022 09:10

brandonwillard reviewed Jul 28, 2022

View reviewed changes

aerubanov closed this by deleting the head repository Jul 29, 2025

		@@ -906,71 +895,6 @@ def validate_grad_graph(func):
		assert softmax_grad_legacy not in ops


		def test_argmax_pushdown():

		@@ -745,400 +733,9 @@ def test_isnan():
		f([[0, 1, 2]])


		class TestMaxAndArgmax:

		@@ -24,28 +22,6 @@
		from tests.link.test_link import make_function


		class TestMaxAndArgmax:

		def R_op(self, inputs, eval_points):
		raise ValueError("Argmax is not a differentiable operation")

Uh oh!

Remove MaxAndArgmax Op #874

Remove MaxAndArgmax Op #874

Uh oh!

Conversation

aerubanov commented Mar 23, 2022 • edited by brandonwillard Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonwillard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aerubanov commented May 2, 2022

Uh oh!

aerubanov commented May 5, 2022

Uh oh!

brandonwillard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aerubanov commented May 31, 2022

Uh oh!

codecov bot commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aerubanov commented Jun 10, 2022

Uh oh!

brandonwillard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aerubanov commented Jul 19, 2022

Uh oh!

aerubanov commented Jul 27, 2022

Uh oh!

brandonwillard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Remove `MaxAndArgmax` `Op` #874

Remove `MaxAndArgmax` `Op` #874

aerubanov commented Mar 23, 2022 •

edited by brandonwillard

Loading

brandonwillard left a comment •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

brandonwillard left a comment •

edited

Loading