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] Use Intable in getitem and setitem consistently #2337

Open
JoeLoser opened this issue Apr 18, 2024 · 3 comments
Open

[stdlib] Use Intable in getitem and setitem consistently #2337

JoeLoser opened this issue Apr 18, 2024 · 3 comments
Labels
good first issue Good for newcomers mojo-repo Tag all issues with this label

Comments

@JoeLoser
Copy link
Collaborator

Many __getitem__ and __setitem__ functions still take an Int as an argument. This is OK, but forces call sites to explicitly construct an Int via int(my_value) for example to call __getitem__ or __setitem__. Instead, we can just make __getitem__ and __setitem__ take an Intable thing that always does the int(value) internally rather than pushing it onto all the callers.

One thing to note is for floating point indices, like x[0.1], this would erroneously call x[int(0.1)] i.e. x[0]. This is surprising and instead should be a hard error. But we can put that on hold and stage this in phases:

  1. Change all the __getitem__ and __setitem__ functions to work on Intable and ignore the floating point indexing issue mentioned above.
  2. Introduce an Integral trait or something to ensure that callers can't pass a floating point value to these functions. Use that instead of Intable everywhere.
  3. Audit other functions using Intable in case they also have floating point indexing issues like mentioned above.

FYI @itramble

@JoeLoser JoeLoser added good first issue Good for newcomers mojo-stdlib Tag for issues related to standard library labels Apr 18, 2024
@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented Apr 18, 2024

The python community already solved this issue of floating points :) See the __index__() docs

This is what is called internally when doing my_list[some_object], this is also why my_list[4.2] won't work in python.

Maybe it would be worth it to create a operator module? Then we can call operator.index(x) instead of x.__index__() since dunder method are not supposed to be public

Some real world examples:

@bgreni
Copy link

bgreni commented Apr 19, 2024

I'd like to work on this if it's still available!

@gabrieldemarmiesse
Copy link
Contributor

@bgreni I don't think you need to ask permission to work on an issue. It's quite uncommon for two contributors to work on the same thing. If you don't see a PR open, it's likely that no one is working on it :)

@linear linear bot removed the mojo-stdlib Tag for issues related to standard library label Apr 29, 2024
@ematejska ematejska added the mojo-repo Tag all issues with this label label Apr 29, 2024
modularbot pushed a commit that referenced this issue May 17, 2024
[External] [stdlib] Add Indexer trait

First half of the split off from #2384.
Partially addresses #2337

Introduces the `Indexer` trait and applies to `Int`, `IntLiteral`,
`Bool`, and integral scalar `SIMD` types.

---------

Co-authored-by: bgreni <42788181+bgreni@users.noreply.github.com>
Closes #2685
MODULAR_ORIG_COMMIT_REV_ID: 224c700e9ff9b28abf9e7d72ff3b3271d7e9de09
msaelices pushed a commit to msaelices/mojo that referenced this issue May 18, 2024
[External] [stdlib] Add Indexer trait

First half of the split off from modularml#2384.
Partially addresses modularml#2337

Introduces the `Indexer` trait and applies to `Int`, `IntLiteral`,
`Bool`, and integral scalar `SIMD` types.

---------

Co-authored-by: bgreni <42788181+bgreni@users.noreply.github.com>
Closes modularml#2685
MODULAR_ORIG_COMMIT_REV_ID: 224c700e9ff9b28abf9e7d72ff3b3271d7e9de09
martinvuyk pushed a commit to martinvuyk/mojo that referenced this issue May 24, 2024
[External] [stdlib] Add Indexer trait

First half of the split off from modularml#2384.
Partially addresses modularml#2337

Introduces the `Indexer` trait and applies to `Int`, `IntLiteral`,
`Bool`, and integral scalar `SIMD` types.

---------

Co-authored-by: bgreni <42788181+bgreni@users.noreply.github.com>
Closes modularml#2685
MODULAR_ORIG_COMMIT_REV_ID: 224c700e9ff9b28abf9e7d72ff3b3271d7e9de09
modularbot pushed a commit that referenced this issue May 28, 2024
[External] [stdlib] Add the `normalize_index` function

This PR is some kind of mix between
#2386 and
#2384 which have issues (aborting)
or have too many conflicts because the PR is too big.

This PR solves part of #2251 and
#2337

We try here to give the ground work for indexing correctly. This
function added can then be used wherever we work with sequences.

Two things I noticed during development:
1) The `debug_assert` does not run in unit tests. Is there any way to
enable it? We currently have out-of-bounds bugs in our test suite.
2) The null terminator is causing pain, again, again, and again. Do we
have any plans to make it optional when working with String? I opened
#2678 to discuss this.

To avoid to fix those issues in this PR, I used the `normalize_index` on
the `__refitem__` of `InlineArray` which doesn't have widespread use yet
and isn't impacted by out-of-bounds bugs.

My recommendation would be to merge this PR then to rebase
#2386 and
#2384 on it. We should also
afterwards fix the out of bounds bugs that can be triggered in the test
suite by enabling debug_assert.

The diff might seem big, but no worries, it's mostly the licenses and
docstrings :)

Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com>
Closes #2677
MODULAR_ORIG_COMMIT_REV_ID: 66e7121a6a333c16284eb33a89eb85c034c296c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

4 participants