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.

Sign up for GitHub

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

Jump to bottom

Rename floor_div to floor_divide to match NumPy's API #1432

Merged
merged 4 commits into from
Feb 27, 2023
Merged

Rename floor_div to floor_divide to match NumPy's API #1432

merged 4 commits into from
Feb 27, 2023

Conversation

sudarsan2k5
Copy link
Contributor

@sudarsan2k5 sudarsan2k5 commented Feb 13, 2023 •
edited
Loading

Addressing #1212

  • Replaced floor_div with floor_divide
  • Deprecated int_div
  • 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.

@codecov
Copy link

codecov bot commented Feb 13, 2023 •
edited
Loading

Codecov Report

Merging #1432 (711b8c6) into main (c433071) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1432      +/-   ##
==========================================
- Coverage   74.86%   74.86%   -0.01%     
==========================================
  Files         194      194              
  Lines       50107    50105       -2     
  Branches    12098    12098              
==========================================
- Hits        37514    37512       -2     
  Misses      10266    10266              
  Partials     2327     2327
Impacted Files Coverage Δ
aesara/compile/profiling.py 74.55% <ø> (ø)
aesara/tensor/basic.py 89.99% <ø> (ø)
aesara/tensor/elemwise.py 88.07% <ø> (ø)
aesara/tensor/var.py 87.82% <50.00%> (ø)
aesara/tensor/rewriting/math.py 86.11% <60.00%> (ø)
aesara/link/jax/dispatch/scalar.py 96.72% <100.00%> (ø)
aesara/link/numba/dispatch/elemwise.py 97.12% <100.00%> (ø)
aesara/scalar/basic.py 79.17% <100.00%> (-0.01%) ⬇️
aesara/tensor/inplace.py 100.00% <100.00%> (ø)
aesara/tensor/math.py 90.67% <100.00%> (-0.01%) ⬇️

brandonwillard
brandonwillard reviewed Feb 13, 2023
View reviewed changes
aesara/tensor/math.py Outdated
@@ -1771,8 +1771,8 @@ def minimum(x, y):


def divmod(x, y):
"""elementvise divmod, using floor_div and mod_check"""
return floor_div(x, y), mod_check(x, y)
"""elementvise divmod, using floor_divide and mod_check"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""elementvise divmod, using floor_divide and mod_check"""
"""Element-wise `divmod`, using `floor_divide` and `mod_check`."""

@sudarsan2k5 sudarsan2k5 reopened this Feb 14, 2023
brandonwillard
brandonwillard previously approved these changes Feb 14, 2023
View reviewed changes
@sudarsan2k5 sudarsan2k5 marked this pull request as draft February 16, 2023 06:15
@sudarsan2k5
Copy link
Contributor Author

I believe there is still some int_div that I need to replace with floor_div and have to add the test for deprecation warning so converting this to draft.

@sudarsan2k5 sudarsan2k5 marked this pull request as ready for review February 16, 2023 11:32
@sudarsan2k5
Copy link
Contributor Author

Hi @brandonwillard,

I made the below changes

  1. replaced floor_div with floor_divide
  2. added an alias int_div to floor_divide
  3. removed some int_div imports and used floor_divide inplace of it
  4. changed some function names and docs to maintain consistency across the code base
  5. renamed true_div_inplace to true_divide_inplace for consistency
  6. added tests for the deprecation of int_div and floor_div

brandonwillard
brandonwillard requested changes Feb 17, 2023
View reviewed changes
tests/tensor/test_math.py Show resolved Hide resolved
aesara/scalar/basic.py Outdated Show resolved Hide resolved
aesara/scalar/basic.py Outdated Show resolved Hide resolved
tests/scalar/test_basic.py Show resolved Hide resolved
aesara/tensor/math.py Outdated Show resolved Hide resolved
aesara/tensor/math.py Outdated Show resolved Hide resolved
@sudarsan2k5
Copy link
Contributor Author

image

Thank you, Now this is working fine

sudarsan2k5
sudarsan2k5 commented Feb 18, 2023
View reviewed changes
aesara/scalar/basic.py Outdated


floor_div = int_div
floor_divide = IntDiv(upcast_out, name="floor_divide")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are renaming everything, should we also rename the class IntDiv, I thing that will make our code consistence

Copy link
Member

Choose a reason for hiding this comment

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

It's a good idea

@sudarsan2k5
Copy link
Contributor Author

Hi @rlouf I have renamed floor_div for floor_divide, added related tests and also renamed to related class name, could you please check if this PR is ready to merge or anything else is still pending? Thanks

@rlouf rlouf changed the title Rename floor_div to floor_divide to match with NumPy Rename floor_div to floor_divide and true_div to true_divide to match with NumPy Feb 27, 2023
@rlouf rlouf changed the title Rename floor_div to floor_divide and true_div to true_divide to match with NumPy Rename floor_div to floor_divide to match NumPy's API Feb 27, 2023
@rlouf
Copy link
Member

rlouf commented Feb 27, 2023

Looks good to me. Thank you for contributing @sudarsan2k5, NumPy compatibility is very important.

@rlouf rlouf merged commit fc937b2 into aesara-devs:main Feb 27, 2023
@sudarsan2k5 sudarsan2k5 deleted the floor_divide branch February 27, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlouf rlouf rlouf left review comments

@brandonwillard brandonwillard brandonwillard requested changes

Assignees
No one assigned
Labels
enhancement New feature or request NumPy compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sudarsan2k5 @rlouf @brandonwillard