Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change use of "optimize" to "rewrite" #1054

Merged
merged 41 commits into from
Aug 17, 2022

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Jul 14, 2022

This PR introduces a lot of refactoring that changes object names and comments to use the term "rewrite" instead of "optimize" when appropriate.

Also, modules with the name opt—or a prefix/suffix of opt—have either been renamed to rewriting or moved to a sub-package with that name.

@brandonwillard brandonwillard self-assigned this Jul 14, 2022
@brandonwillard brandonwillard added documentation Improvements or additions to documentation important refactor This issue involves refactoring labels Jul 14, 2022
@brandonwillard brandonwillard force-pushed the graph-opt-refactoring branch 2 times, most recently from ff1bdcb to 658c844 Compare July 14, 2022 23:12
@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #1054 (539addb) into main (e0d9180) will decrease coverage by 0.03%.
The diff coverage is 80.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
- Coverage   79.28%   79.25%   -0.04%     
==========================================
  Files         151      159       +8     
  Lines       47992    48097     +105     
  Branches    10913    10934      +21     
==========================================
+ Hits        38052    38117      +65     
- Misses       7437     7469      +32     
- Partials     2503     2511       +8     
Impacted Files Coverage Δ
aesara/configdefaults.py 66.20% <ø> (ø)
aesara/graph/features.py 64.69% <ø> (ø)
aesara/graph/fg.py 87.96% <ø> (ø)
aesara/graph/rewriting/basic.py 64.96% <ø> (ø)
aesara/sandbox/multinomial.py 76.51% <ø> (+0.60%) ⬆️
aesara/scan/opt.py 0.00% <0.00%> (-79.00%) ⬇️
aesara/sparse/opt.py 0.00% <0.00%> (-75.52%) ⬇️
aesara/tensor/basic_opt.py 100.00% <ø> (+14.30%) ⬆️
aesara/tensor/math_opt.py 100.00% <ø> (+12.85%) ⬆️
aesara/tensor/nnet/abstract_conv.py 76.66% <ø> (ø)
... and 44 more

ricardoV94
ricardoV94 previously approved these changes Jul 15, 2022
Copy link
Contributor

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the new names sound much more intuitive now.

@brandonwillard
Copy link
Member Author

Some of the new names sound much more intuitive now.

Yeah, I was going for something systematic and explanatory, because even I have trouble remembering (by name) what these things do.

@brandonwillard
Copy link
Member Author

@aesara-devs/aesara, all right, I'm going to merge this, which means that we'll need to update the other aesara-devs projects and PyMC in order to remove the new deprecation warnings.

@michaelosthege
Copy link
Contributor

@aesara-devs/aesara, all right, I'm going to merge this, which means that we'll need to update the other aesara-devs projects and PyMC in order to remove the new deprecation warnings.

👍

This is going to be Aesara 2.8.0 then?

@brandonwillard
Copy link
Member Author

This is going to be Aesara 2.8.0 then?

Yes, definitely.

@oscarbenjamin
Copy link

It seems this makes some deprecated names unavailable from the packages where they were previously available e.g.:

In [1]: import aesara

In [2]: aesara.tensor
Out[2]: <module 'aesara.tensor' from '/home/oscar/current/sympy/sympy.git/38venv/lib/python3.8/site-packages/aesara/tensor/__init__.py'>

In [3]: aesara.tensor.abs_
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-f120198d92d4> in <module>
----> 1 aesara.tensor.abs_

AttributeError: module 'aesara.tensor' has no attribute 'abs_'

In [4]: aesara.tensor.math.abs_
<ipython-input-4-8f09823c3c05>:1: DeprecationWarning: `abs_` is deprecated; use `abs` instead.
  aesara.tensor.math.abs_
Out[4]: <aesara.tensor.elemwise.Elemwise at 0x7fa9c1990eb0>

That leads to a test failure in SymPy CI due to this line:
https://github.com/sympy/sympy/blob/88664e6e0b781d0a8b5347896af74b555e92891e/sympy/printing/aesaracode.py#L22

Is it always safe to use abs insteaed of abs_ including with older versions of Aesara and also Theano?

@brandonwillard
Copy link
Member Author

Is it always safe to use abs insteaed of abs_ including with older versions of Aesara and also Theano?

It's not safe for Theano and only up to 430d068 in Aesara.

@brandonwillard
Copy link
Member Author

@oscarbenjamin, we can probably add deprecation support for aesara.tensor.abs_, if that helps. Regardless, thanks for keeping up on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation important refactor This issue involves refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants