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

[stdlib] Add unit tests for the SIMD __rfloordiv__() method and constraint check on the element types #2414

Closed
wants to merge 1 commit into from
Closed

[stdlib] Add unit tests for the SIMD __rfloordiv__() method and constraint check on the element types #2414

wants to merge 1 commit into from

Conversation

peymanbr
Copy link

The rfloordiv() method is already supported by Int and Object. This commit adds support for this method to the SIMD type.

@peymanbr peymanbr requested a review from a team as a code owner April 26, 2024 14:34
@peymanbr peymanbr mentioned this pull request Apr 26, 2024
Closed
1 task
gabrieldemarmiesse
gabrieldemarmiesse reviewed Apr 26, 2024
View reviewed changes
stdlib/test/builtin/test_simd.mojo Outdated
@@ -102,6 +102,24 @@ def test_floordiv():
assert_equal(Float32(2) // Float32(-2), -1)
assert_equal(Float32(99) // Float32(-2), -50)

assert_equal(
SIMD[DType.int32, 4](2, 4, 1, 5) // Int32(2),
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this actually calling __floordiv__ instead of __rfloordiv__? Since the compiler will try __floordiv__ first if the two elements are both SIMD. This raises the question, in what situation is this implementation of __rfloordiv__ useful? In python, it's only used to handle other types.

Maybe there is some compiler logic I'm missing here?

Copy link
Author

Choose a reason for hiding this comment

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

Correct that test and the one right below it call floordiv(). I added those tests to have more test coverage for floordiv(). I can exclude them in this PR and add them in another PR if its misleading.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Then I have a follow-up question: in which situation is it possible to call the function__rfloordiv__ that was just added?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can happen if there is some implicit conversion. It can also be required by some traits (e.g. CeilDivable). See #2414 (comment)

peymanbr
peymanbr commented Apr 26, 2024
View reviewed changes
stdlib/src/builtin/simd.mojo Outdated
The element type of the SIMD vector must be numeric.

Args:
value: The value to divide on.
Copy link
Author

Choose a reason for hiding this comment

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

Change to "The value that is being divided."

abduld
abduld approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@abduld abduld left a comment

Choose a reason for hiding this comment

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

Thanks

@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented Apr 30, 2024 •
edited

If I may add, unless I'm missing something, the method __rfloordiv__ added in this PR can't be called. A call to // between two SIMD values will always call __floordiv__. Thus while the unit tests added in this PR are good and can be merged, the added method might not be as useful as we previously thought.

@peymanbr peymanbr force-pushed the add_rfloordiv_to_simd branch 2 times, most recently from 919f9c9 to ec773b8 Compare May 3, 2024 17:41
@peymanbr peymanbr changed the title Add __rfloordiv__ to the SIMD type [stdlib] Add __rfloordiv__ to the SIMD type May 3, 2024
@peymanbr peymanbr force-pushed the add_rfloordiv_to_simd branch 2 times, most recently from c867d15 to 7b91b90 Compare May 3, 2024 18:21
@peymanbr
Copy link
Author

peymanbr commented May 6, 2024 •
edited

The added __rfloordiv__() method is definitely called; otherwise, the added unit tests in the PR would fail without it.

@ematejska ematejska added the mojo-repo Tag all issues with this label label May 6, 2024
@JoeLoser JoeLoser added the imported-internally Signals that a given pull request has been imported internally. label May 7, 2024
@gabrieldemarmiesse
Copy link
Contributor

It seems that __rfloordiv__ was already added by an internal commit.

See https://github.com/modularml/mojo/blob/nightly/stdlib/src/builtin/simd.mojo#L661

It came from this commit: f5aee40

@peymanbr I'm sorry that this was already done by someone else. Coordination is pretty hard on a project evolving as fast as Mojo. Maybe it should be possible to change this PR to only add unit tests? That should still be a great addition :)

@peymanbr peymanbr changed the title [stdlib] Add __rfloordiv__ to the SIMD type [stdlib] Add unit tests for the SIMD __rfloordiv__() method and constraint check on the element types May 7, 2024
@peymanbr
Copy link
Author

peymanbr commented May 7, 2024

@gabrieldemarmiesse I understand that there are a lot of activities in this repo. Thank you for notifying me about the added functionality.

I think the constraint check on the element types is missing in the implementation. Therefore, I've added this check to the __floordiv__() method implementation and limited the PR to include this check and the unit tests. I've also updated the title of this issue to accurately reflect the changes made in the PR.

@laszlokindrat
Copy link
Contributor

Thanks for the patch! I had to do some refactoring of the math module, and ended up implementing SIMD.__rfloordiv__, sorry about that! I do however would appreciate a patch with tests for this, and you almost have them. Can you update them, so they directly use the __rfloordiv__ function call instead of the // operator? This should be the preferred way to unittest these magic methods for the basic numeric types (because it is less likely that a test would accidentally pass due to some implicit conversion rules).

@peymanbr
Add unit tests for the SIMD __rfloordiv__() method
4a432f5 
The support for __rfloordiv__() with SIMD types was added in:
modularml@f5aee40

This commit adds:
  - unit tests for __rfloordiv__() in SIMD types.
  - the constraint check to the method's implementation.

Signed-off-by: Peyman Barazandeh <peymanb@gmail.com>
@soraros
Copy link
Contributor

soraros commented May 7, 2024

@laszlokindrat I thought the reason we need the r variant because we always implicit conversion to work. I think it's valuable to also have tests excising the operator path.

@peymanbr
Copy link
Author

peymanbr commented May 7, 2024

@laszlokindrat @gabrieldemarmiesse Thank you for your feedback.

I changed the added tests to call __rfloordiv__ function instead of the // operator and moved them to their own test function named test_rfloordiv() for better visibility.

@laszlokindrat
Copy link
Contributor

@laszlokindrat I thought the reason we need the r variant because we always implicit conversion to work. I think it's valuable to also have tests excising the operator path.

Yes, but we need to figure a more robust way to ensure that these tests actually hit that conversion path. Although these are less important because the compiler has internal tests to ensure it resolves the magic methods in the correct precedence order. Either way, we need the method itself to have unit tests that aren't affected by conversion rules. I will try to write up some guidelines on this for the future, but for now, just ensure you test dunder methods directly for the core numeric types (and maybe others as well).

laszlokindrat
laszlokindrat approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@laszlokindrat laszlokindrat left a comment

Choose a reason for hiding this comment

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

Thanks!

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added the merged-internally Indicates that this pull request has been merged internally label May 7, 2024
JoeLoser pushed a commit that referenced this pull request May 8, 2024
@peymanbr @JoeLoser
[External] [stdlib] Add unit tests for the SIMD __rfloordiv__() metho…
e17e33c 
…d and constraint check on the element types (#39451)

[External] [stdlib] Add unit tests for the SIMD __rfloordiv__() method
and constraint check on the element types

The __rfloordiv__() method is already supported by Int and Object. This
commit adds support for this method to the SIMD type.

Co-authored-by: Peyman Barazandeh <peymanb@gmail.com>
Closes #2414
MODULAR_ORIG_COMMIT_REV_ID: d1050970b5e965aa2df06e9e1378763eaef61551
@JoeLoser JoeLoser added the merged-externally Merged externally in public mojo repo label May 8, 2024
@JoeLoser JoeLoser closed this May 8, 2024
@peymanbr peymanbr deleted the add_rfloordiv_to_simd branch May 9, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrieldemarmiesse gabrieldemarmiesse gabrieldemarmiesse left review comments

@abduld abduld abduld approved these changes

@laszlokindrat laszlokindrat laszlokindrat approved these changes

Assignees
No one assigned
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally mojo-repo Tag all issues with this label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@peymanbr @gabrieldemarmiesse @laszlokindrat @soraros @modularbot @abduld @JoeLoser @ematejska