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

Update __truediv__ comment in python bindings. #3586

Merged
merged 4 commits into from
Dec 14, 2024
Merged

Update __truediv__ comment in python bindings. #3586

merged 4 commits into from
Dec 14, 2024

Conversation

rdspring1
Copy link
Collaborator

This PR updates the comments for __truediv__ operator defined in python bindings. The current comment does not reflect what the code actually does.

Reference: #2837 (review)

@rdspring1 rdspring1 added the Python API Issues related to the Python API label Dec 13, 2024
@rdspring1 rdspring1 requested a review from jjsjann123 December 13, 2024 01:47
@rdspring1
Copy link
Collaborator Author

!build

// __div__.
// In python, __truediv__ (/) always returns a float regardless of whether
// the input arguments are float or integer. __truediv__ (/) corresponds with
// pytorch torch.true_divide(a, b). The __div__ operator is depreciated in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// pytorch torch.true_divide(a, b). The __div__ operator is depreciated in
// pytorch torch.true_divide(a, b). The __div__ operator is deprecated in

Comment on lines 2064 to 2068
// In nvfuser, truediv function has the same semantics as python's
// operator __truediv__ (/). The div function truncates the result instead
// of promoting it to float, so it has the same semantics as the C++'s (/)
// operator. In pytorch, torch.div(a, b, rounding_mode='trunc') corresponds
// C-style integer division.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first sentence here is incorrect I think. On Python 3.12, __truediv__ of ints promotes to double first and does no truncation:

[ins] In [1]: x = 3

[ins] In [2]: y = 4

[ins] In [3]: x / y
Out[3]: 0.75

[ins] In [4]: x.__truediv__(4)
Out[4]: 0.75

To get truncdiv in Python I think you need to manually truncate:

[ins] In [17]: ((-30)/4).__trunc__()
Out[17]: -7

[ins] In [18]: (-30)//4
Out[18]: -8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How is the first sentence incorrect? It is what the comment says in csrc/ops/arith.h

// truediv: promote to float for integer division, has the same semantics as the
// python's operator /

https://github.com/NVIDIA/Fuser/blob/main/csrc/ops/arith.h#L426-L431

// div: don't promote to float, instead, truncate the result, this has the same
// semantics as the C++'s operator /

https://github.com/NVIDIA/Fuser/blob/main/csrc/ops/arith.h#L432-L437

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the comment to directly mention csrc/ops/arith.h.

We are changing the dunder __truediv__ in our python API to map to c-style division.

Copy link
Collaborator

@jacobhinkle jacobhinkle left a comment

Choose a reason for hiding this comment

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

LGTM!

// python's operator __truediv__ (/). The div function in csrc/ops/arith.h
// truncates the result instead of promoting it to float. It has the same
// semantics as the C++'s (/) operator. In pytorch,
// torch.div(a, b, rounding_mode='trunc') corresponds C-style integerw
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// torch.div(a, b, rounding_mode='trunc') corresponds C-style integerw
// torch.div(a, b, rounding_mode='trunc') corresponds C-style integer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I blame vim. 😄

@rdspring1
Copy link
Collaborator Author

!build

@rdspring1 rdspring1 merged commit b68a7e4 into main Dec 14, 2024
17 checks passed
@rdspring1 rdspring1 deleted the truediv_type branch December 14, 2024 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python API Issues related to the Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants