-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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] Implement Int.from_bytes()
and Int.as_bytes()
#3795
Closed
Closed
Changes from 18 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
fc608a7
Implement Int.from_bytes() function
msaelices cd91096
Add entry to the changelog about Int.from_bytes()
msaelices 3eca741
Make the Int.from_bytes() accept an span of bytes instead of a list
msaelices b9f189d
[stdlib] Move the Int.from_bytes() big_endian arg to a comptime param
msaelices cc97ffe
[stdlib] Implement Int.as_bytes()
msaelices b580bc9
[stdlib] Update changelog to include Int.as_bytes()
msaelices 569e6ff
[stdlib] Simplify is_big_endian import
msaelices 239bba6
Optimize copying bytes into the array as we know it's a trivial type
msaelices 21384ec
Merge branch 'nightly' into int-from-and-to-bytes
msaelices bc22870
Merge branch 'nightly' into int-from-and-to-bytes
msaelices 63fa5b4
Infer the Span type based on DType
msaelices 5b41e1f
Merge branch 'nightly' into int-from-and-to-bytes
msaelices b5e09c5
[stdlib] Revert part of the changes here as compiler does not support…
msaelices 7766ea3
[stdlib] Move the Bytes alias in the test closer to the scope we use it
msaelices 30027df
[stdlib] Less usage of UnsafePointer in the Int.from_bytes|as_bytes
msaelices eed9463
More meaningful test function name
msaelices 7f142a8
Merge branch 'nightly' into int-from-and-to-bytes
msaelices 3b93cd6
Fix issue in the previous merge conflict, as we need the Span
msaelices b0bc485
Merge branch 'nightly' into int-from-and-to-bytes
msaelices 776b4df
Move the logic to SIMD so we can call it with whatever scalar we want
msaelices 265b648
Move the Int.as_bytes() logic to SIMD, so we can call it with any sca…
msaelices 0e5d293
Add reference ti new SIMD methods to the changelog
msaelices 6e144d5
Tests for SIMD.from_bytes and SIMD.as_bytes
msaelices 3211d24
Remove uneeded import sentences in int.mojo
msaelices 8caad41
Try to not allocating memory by using InlineArray instead of List
msaelices dd29814
Fix the call expansion failed issue. Adapt unit tests
msaelices 694693a
Merge branch 'nightly' into int-from-and-to-bytes
msaelices 40b9fc0
Remove the raises declaration from Int.from_bytes() and SIMD.from_byt…
msaelices 98c29fc
Remove the DType parameter from Int.from_bytes() and Int.as_bytes()
msaelices 8db9a83
Merge branch 'nightly' into int-from-and-to-bytes
msaelices 3af72c5
No need for the Span here
msaelices File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I don't think my comment before was ever properly addressed. I think this method should be on
SIMD
, notInt
. That way theDType
makes more sense. We can think more about what it would look like onInt
later, and doingint(UInt64.from_bytes(span))
doesn't seem like too much of a hit to me.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.
Sorry, I missed it. Done:
msaelices@776b4df
msaelices@265b648
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.
Sorry for all the change requests, but to be clear I am comfortable merging these changes on
SIMD
, but not onInt
. I think theSIMD
API makes a ton of sense, but theint
API would need some more thought since taking aDType
feels wrong.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.
Agreed, I think we should/could land the uncontroversial part (
from/to_byte
onSIMD
) first and iterate. WDYT @msaelices?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.
Yes, actually the whole point of this PR was to help Python developers who need to migrate Python code using int.from_bytes() and int.to_bytes(). So, to me, it's easier if we have it on the
Int
struct too, and iterate it.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.
I disagree with having it on the Int struct too because we would either:
I'm also not compelled by the idea that the goal is to mirror a Python API considering your earlier argument in favor of breaking away from the Python API using a parameter. As I mentioned before, I think
int(UInt64.from_bytes(my_span))
is fine for now