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

[stdlib] Add __repr__ in SIMD #2728

Closed
wants to merge 1 commit into from
Closed

Conversation

bgreni
Copy link

@bgreni bgreni commented May 18, 2024

Implements __repr__ for SIMD

@bgreni bgreni requested a review from a team as a code owner May 18, 2024 00:05
@laszlokindrat laszlokindrat self-assigned this May 18, 2024
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, this could be a good improvement! I wonder what we should do with floating point values. We looked at this recently with @ConnorGray for numpy, but I can't remember what we concluded. Since we don't have good utilities for controlling floating point printing precision globally (the way numpy does), it might be safest to print all useful digits (the number of which depends on the float type), and use scientific notation.

Either way, please add a changelog entry.

stdlib/src/builtin/simd.mojo Outdated Show resolved Hide resolved
stdlib/test/builtin/test_simd.mojo Show resolved Hide resolved
stdlib/test/builtin/test_simd.mojo Show resolved Hide resolved
@laszlokindrat
Copy link
Contributor

Could you please rebase this after tomorrow's nightly? Also, I really think the implementation should be verbose here, and print all significant digits and use scientific notation.

@bgreni
Copy link
Author

bgreni commented May 21, 2024

Could you please rebase this after tomorrow's nightly? Also, I really think the implementation should be verbose here, and print all significant digits and use scientific notation.

Sorry could you maybe give an example of the output you're expecting?

@laszlokindrat
Copy link
Contributor

Something like this:

assert_equal(
    SIMD[DType.float32, 1](1232.4).__repr__(),
    "SIMD[DType.float32, 1](1.2324000e+03)",  #  8 digits for float32, 16 for float64, etc
)

Looking at this now, I'm not actually sure about the trailing zeros, but in repr, I would prefer scientific notation.

@bgreni
Copy link
Author

bgreni commented May 23, 2024

Ok I'll do that thanks!

@bgreni bgreni force-pushed the simd-repr branch 3 times, most recently from f5d3e45 to 53ca34b Compare May 23, 2024 15:57
@bgreni
Copy link
Author

bgreni commented May 23, 2024

@laszlokindrat I've got the scientific notation working, hopefully you don't find the required changes too invasive. For now I haven't worried about trailing zeroes until you've made a concrete decision on that front.

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 for pushing this forward! The behavior with respect to trailing zeros is fine for now; we can iterate later if needed. Could you please rebase and ensure CI is happy?

docs/changelog.md Outdated Show resolved Hide resolved
stdlib/src/builtin/dtype.mojo Show resolved Hide resolved
stdlib/src/builtin/io.mojo Show resolved Hide resolved
stdlib/src/builtin/simd.mojo Outdated Show resolved Hide resolved
stdlib/src/builtin/simd.mojo Outdated Show resolved Hide resolved
Signed-off-by: Brian Grenier <grenierb96@gmail.com>
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.

Awesome, thanks!

@laszlokindrat
Copy link
Contributor

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 27, 2024
@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 merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels May 27, 2024
@modularbot
Copy link
Collaborator

Landed in ec6ecf9! Thank you for your contribution 🎉

modularbot pushed a commit that referenced this pull request May 28, 2024
[External] [stdlib] Add `__repr__` in `SIMD`

Implements `__repr__` for `SIMD`

---------

Co-authored-by: bgreni <42788181+bgreni@users.noreply.github.com>
Closes #2728
MODULAR_ORIG_COMMIT_REV_ID: 53688198a78f892e2d4799bc8bec3f7974a538fb
@modularbot modularbot closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants