Remove `MaxAndArgmax` `Op` by aerubanov · Pull Request #874 · aesara-devs/aesara

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

Closes #765

brandonwillard

Looks like this is making good progress; much appreciated!

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

-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

            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

        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

 from tests.link.test_link import make_function


-class TestMaxAndArgmax:


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

+            # 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

+    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 <971601+brandonwillard@users.noreply.github.com>

Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.noreply.github.com>

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

-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

-        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

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

brandonwillard suggested changes May 1, 2022

View reviewed changes

brandonwillard reviewed May 27, 2022

View reviewed changes

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 suggested changes Jul 16, 2022

View reviewed changes

aerubanov and others added 3 commits July 18, 2022 18:38

Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.norep…

ab2b87e

…ly.github.com> modify Max Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.noreply.github.com>

modify Argmax

ccf42be

Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.noreply.github.com>

remove MaxAndArgmax

114bf01

Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.noreply.github.com>

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

		assert softmax_grad_legacy not in ops


		def test_argmax_pushdown():

		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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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