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

Rename true_div to true_divide to match with NumPy #1414

Merged
merged 6 commits into from Feb 13, 2023

Conversation

sudarsan2k5
Copy link
Contributor

Addressing #1212

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

@rlouf
Copy link
Member

rlouf commented Feb 7, 2023

Thank you for contributing! I think we'll need to alias true_div to not break the code that currently relies on this, and add a deprecation warning.

@sudarsan2k5
Copy link
Contributor Author

Hi @rlouf I have added an alias for true_division but could you please guide me how to set deprecation warning for an alias? I know how to do with Function but not sure for an alias/variable.

@rlouf
Copy link
Member

rlouf commented Feb 8, 2023

The alias is a function, and you can add it to the DEPRECATED_NAMES list at the end of the module.

PS: it's only a small change to add the at.divide alias for the new at.true_divide so we should add it here as well.

@sudarsan2k5
Copy link
Contributor Author

Hi @rlouf, as you suggested, I have created an alias divide to true_divide and deprecated true_div.

Could you please check that I did everything correctly because there is no deprecation warning while importing it?

Is there anything else we need to do for true_div in this PR? Also, I'm not sure how to fix the failed cases; please advise.

Is it also necessary to rename the class TrueDiv to TrueDivide?

aesara/scalar/basic.py Outdated Show resolved Hide resolved
aesara/scalar/basic.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #1414 (1354dab) into main (a5008d1) will increase coverage by 0.00%.
The diff coverage is 80.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1414   +/-   ##
=======================================
  Coverage   74.72%   74.72%           
=======================================
  Files         194      194           
  Lines       49866    49868    +2     
  Branches    10549    10549           
=======================================
+ Hits        37262    37264    +2     
  Misses      10274    10274           
  Partials     2330     2330           
Impacted Files Coverage Δ
aesara/tensor/elemwise.py 88.07% <ø> (ø)
aesara/tensor/nnet/basic.py 32.69% <33.33%> (ø)
aesara/tensor/nnet/batchnorm.py 15.38% <50.00%> (ø)
aesara/tensor/rewriting/math.py 85.95% <76.19%> (ø)
aesara/scalar/basic.py 79.04% <100.00%> (+0.01%) ⬆️
aesara/scalar/math.py 85.00% <100.00%> (ø)
aesara/tensor/math.py 90.42% <100.00%> (ø)
aesara/tensor/rewriting/special.py 77.77% <100.00%> (ø)
aesara/tensor/var.py 87.82% <100.00%> (ø)

@brandonwillard
Copy link
Member

Thanks @sudarsan2k5!

@brandonwillard brandonwillard merged commit 4961799 into aesara-devs:main Feb 13, 2023
@sudarsan2k5 sudarsan2k5 deleted the division_op branch February 13, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NumPy compatibility refactor This issue involves refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The naming of the division Ops is inconsistent with NumPy's
3 participants