-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[stdlib] Add unit tests for the SIMD __rfloordiv__() method and constraint check on the element types #2414
Conversation
5291ec9
to
3075a7d
Compare
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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
stdlib/src/builtin/simd.mojo
Outdated
The element type of the SIMD vector must be numeric. | ||
|
||
Args: | ||
value: The value to divide on. |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
If I may add, unless I'm missing something, the method |
919f9c9
to
ec773b8
Compare
c867d15
to
7b91b90
Compare
The added |
It seems that 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 :) |
7b91b90
to
8c71759
Compare
@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 |
8c71759
to
7dcb1e1
Compare
Thanks for the patch! I had to do some refactoring of the |
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>
7dcb1e1
to
4a432f5
Compare
@laszlokindrat I thought the reason we need the |
@laszlokindrat @gabrieldemarmiesse Thank you for your feedback. I changed the added tests to call |
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
✅🟣 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. |
…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
…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 modularml#2414 MODULAR_ORIG_COMMIT_REV_ID: d1050970b5e965aa2df06e9e1378763eaef61551 Signed-off-by: Lukas Hermann <lukashermann28@gmail.com>
The rfloordiv() method is already supported by Int and Object. This commit adds support for this method to the SIMD type.