Deprecate `nnet` and `signal` modules by rlouf · Pull Request #1188 · aesara-devs/aesara

rlouf · 2022-09-15T20:30:05Z

In this PR we begin phasing the nnet and signal modules outside of Aesara, following discussion in #674.

Anything else we want to salvage ?

codecov · 2022-09-16T09:58:07Z

Codecov Report

Merging #1188 (aed5986) into main (739bd49) will decrease coverage by 5.05%.
The diff coverage is 86.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1188      +/-   ##
==========================================
- Coverage   79.15%   74.09%   -5.06%     
==========================================
  Files         173      174       +1     
  Lines       48536    48605      +69     
  Branches    10322    10340      +18     
==========================================
- Hits        38417    36014    -2403     
- Misses       7628    10305    +2677     
+ Partials     2491     2286     -205

Impacted Files	Coverage Δ
aesara/tensor/signal/conv.py	`0.00% <0.00%> (-75.68%)`	⬇️
aesara/tensor/rewriting/special.py	`77.77% <77.77%> (ø)`
aesara/tensor/special.py	`90.64% <90.64%> (ø)`
aesara/link/jax/dispatch/elemwise.py	`80.59% <100.00%> (ø)`
aesara/link/numba/dispatch/elemwise.py	`97.12% <100.00%> (ø)`
aesara/scan/op.py	`85.07% <100.00%> (-0.34%)`	⬇️
aesara/tensor/nnet/basic.py	`32.61% <100.00%> (-47.67%)`	⬇️
aesara/tensor/signal/pool.py	`28.30% <100.00%> (-40.97%)`	⬇️
aesara/tensor/nnet/neighbours.py	`0.00% <0.00%> (-91.27%)`	⬇️
aesara/tensor/nnet/conv3d2d.py	`0.00% <0.00%> (-81.16%)`	⬇️
... and 25 more

twiecki · 2022-09-16T13:41:39Z

I would vote to also keep conv2d around, as convolutions are super common in all kinds of areas. I'd actually love to have a conv1d also, as there were too many cases where we had to roll our own.

brandonwillard · 2022-09-16T16:01:12Z

Anything else we want to salvage ?

Looks good to me.

rlouf · 2022-09-26T08:52:24Z

Anything else we want to salvage ?

Looks good to me.

Just to be clear, do you mean that we only keep Softmax, LogSoftmax (which should be expressed as an Aesara graph + rewrites instead but that's another PR) and SoftmaxGrad and remove everything else?

ricardoV94 · 2022-09-26T09:28:57Z

Agree with @twiecki to keep the conv2d Op until we find some place to move it to. Everything else seems very specialized, easy to recreate.

rlouf · 2022-09-26T09:32:05Z

We have the deprecation period to move it somewhere else, and it's better to raise a warning while we're considering moving it so downstream callers are not surprised the day it disappears.

(In this case we'll also need to update the warning when it is moved elsewhere)

rlouf · 2022-09-29T20:49:22Z

I have moved Softmax, LogSoftmax and SoftmaxGrad to aesara.tensor.math, but it may be better to create a new aesara.tensor.special module for these functions to have a familiar import path.

rlouf · 2022-09-29T21:30:08Z

Alright went through the checklist, will take another good look (especially at the documentation) tomorrow and move softmax-related Ops and functions to aesara.tensor.special. Then it should be good to merge once tests pass here.

Also, I indicated rel-2.9.0 as the deadline after which everything will be deleted. Does that sound right?

brandonwillard · 2022-09-29T22:02:00Z

Also, I indicated rel-2.9.0 as the deadline after which everything will be deleted. Does that sound right?

Yeah, that sounds fine.

twiecki · 2022-09-30T13:35:37Z

So it seems like conv2d is being dropped?

rlouf · 2022-09-30T14:10:38Z

I'd actually love to have a conv1d also, as there were too many cases where we had to roll our own.

We can open an issue to implement the equivalent of np.convolve. That's in scope.

As for conv_2d the question is more: why should we keep it in Aesara?

rlouf · 2022-09-30T14:17:52Z

Opened an issue for 1d convolution: #1223

twiecki · 2022-09-30T14:19:48Z

conv_2d the question is more: why should we keep it in Aesara?

Don't have a good reason other than that it's useful to some PyMC models. The alternative is to add this to pymc/aesaraf.py, which is a fine solution too.

brandonwillard · 2022-09-30T19:55:30Z

conv_2d the question is more: why should we keep it in Aesara?

Don't have a good reason other than that it's useful to some PyMC models. The alternative is to add this to pymc/aesaraf.py, which is a fine solution too.

I believe the original idea was to create a separate project and move all this DL-specific code there. We can always do that. We can also keep conv2d for the time being, especially if removing it will break things in PyMC.

rlouf · 2022-09-30T20:18:15Z

I believe the original idea was to create a separate project and move all this DL-specific code there.

Yeah that would be the ideal. You will still be able to import conv_2d after this PR is merged, and we can use the deprecation period to find a solution that works for everyone.

brandonwillard

Is there anything left on this one? If not, we can probably merge this.

brandonwillard · 2022-10-05T19:00:30Z

I am currently excluding all the test related to aesara.tensor.nnet and aesara.tensor.signal. I can either include the tests related to conv2d individually, or use pytests.mark.skip on the others. I think refactoring the nnet and signal test suite is out of scope for this PR, and even for Aesara now that they're getting deprecated.

What is your preferred solution?

We can include them individually, if that's not too involved.

rlouf · 2022-10-12T14:25:27Z

@brandonwillard I included the conv2d tests individidually, I think we can merge this if the tests pass.

@twiecki I could not find where conv2d is used in PyMC. Would you mind opening an issue there regarding the deprecation of this Op in Aesara so you can start porting the code?

rlouf · 2022-10-13T11:38:19Z

Codecov warnings are about tests in nnet and signal, I think this can be reviewed one last time and merged.

… documentation

…cial`

brandonwillard · 2022-10-17T21:25:33Z

aesara/scan/op.py



-@op_debug_information.register(Scan)
+@op_debug_information.register(Scan)  # type: ignore[has-type]


This looks like it's in response to #1221.

This was linked to issues Sep 15, 2022

Remove/rename aesara.tensor.nnet sub-package #674

Closed

Move Softmax and related Ops out of nnet module #764

Closed

brandonwillard added refactor This issue involves refactoring important labels Sep 16, 2022

rlouf force-pushed the deprecate-nn branch from d3f88c8 to e995ece Compare September 29, 2022 21:26

rlouf force-pushed the deprecate-nn branch 2 times, most recently from 3c091da to aff3ddd Compare September 29, 2022 21:32

rlouf force-pushed the deprecate-nn branch 2 times, most recently from 2b385e0 to 01f121c Compare September 30, 2022 18:09

rlouf force-pushed the deprecate-nn branch 2 times, most recently from 3420c24 to 799ba2f Compare September 30, 2022 20:14

rlouf force-pushed the deprecate-nn branch 4 times, most recently from cdabfd6 to 9408c14 Compare October 1, 2022 12:29

rlouf force-pushed the deprecate-nn branch from 04a3e25 to 4f20c7f Compare October 5, 2022 14:32

brandonwillard reviewed Oct 5, 2022

View reviewed changes

brandonwillard force-pushed the deprecate-nn branch from 4f20c7f to 3639bfb Compare October 5, 2022 18:49

brandonwillard assigned brandonwillard and rlouf and unassigned brandonwillard Oct 5, 2022

brandonwillard added this to the Clean up after Theano milestone Oct 5, 2022

rlouf force-pushed the deprecate-nn branch from f3c3826 to c9de24e Compare October 12, 2022 14:24

rlouf requested a review from brandonwillard October 14, 2022 08:03

rlouf added 7 commits October 17, 2022 21:57

Ignore nnet and signal modules during tests

c3e1904

Remove automatic import of nnet module

3e232b4

Add deprecation warning in nnet module

09dbf06

Add deprecation warnings in signal module

5dcda4f

Remove module aesara.sandbox.neighbour redirecting to nnet

483190c

Move Sotfmax, SoftmaxGrad LogSoftmax to aesara.tensor.math

5f11bdc

Ignore type error in scan

3ea559c

rlouf force-pushed the deprecate-nn branch from c9de24e to d2731db Compare October 17, 2022 20:00

rlouf added 5 commits October 17, 2022 22:42

Remove mentions of aesara.tensor.signal and aesara.tensor.nnet in…

45047d3

… documentation

Remove use of aesara.tensor.nnet in other tests

91da535

Move Softmax, LogSoftmax, SoftmaxGrad to new `aesara.tensor.spe…

b2c4dae

…cial`

Fix the rewrite tests

0f85075

Add conv2d to tests

aed5986

rlouf force-pushed the deprecate-nn branch from 89a3419 to aed5986 Compare October 17, 2022 20:43

brandonwillard reviewed Oct 17, 2022

View reviewed changes

brandonwillard approved these changes Oct 17, 2022

View reviewed changes

brandonwillard merged commit aa5223c into aesara-devs:main Oct 17, 2022

jessegrabowski mentioned this pull request Oct 28, 2022

Add example on using BVAR model for economists pymc-devs/pymc-examples#286

Closed



		@op_debug_information.register(Scan)
		@op_debug_information.register(Scan) # type: ignore[has-type]

Uh oh!

Conversation

rlouf commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

twiecki commented Sep 16, 2022 • edited by rlouf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonwillard commented Sep 16, 2022

Uh oh!

rlouf commented Sep 26, 2022

Uh oh!

ricardoV94 commented Sep 26, 2022

Uh oh!

rlouf commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlouf commented Sep 29, 2022

Uh oh!

rlouf commented Sep 29, 2022

Uh oh!

brandonwillard commented Sep 29, 2022

Uh oh!

twiecki commented Sep 30, 2022

Uh oh!

rlouf commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlouf commented Sep 30, 2022

Uh oh!

twiecki commented Sep 30, 2022

Uh oh!

brandonwillard commented Sep 30, 2022

Uh oh!

rlouf commented Sep 30, 2022

Uh oh!

brandonwillard left a comment

Choose a reason for hiding this comment

Uh oh!

brandonwillard commented Oct 5, 2022

Uh oh!

rlouf commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlouf commented Oct 13, 2022

Uh oh!

brandonwillard Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rlouf commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading

twiecki commented Sep 16, 2022 •

edited by rlouf

Loading

rlouf commented Sep 26, 2022 •

edited

Loading

rlouf commented Sep 30, 2022 •

edited

Loading

rlouf commented Oct 12, 2022 •

edited

Loading