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

Refactoring to include index_nplike and reducers #1490

Merged
merged 10 commits into from
Jun 8, 2022

Conversation

findingmochi
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jun 6, 2022

@findingmochi findingmochi changed the title The new JAXNumpyArray class Refactoring to include index_nplike Jun 6, 2022
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I like this approach!

@agoose77
Copy link
Collaborator

agoose77 commented Jun 7, 2022

I'm a 👍 on this as well, it will be good to formally distinguish between index and content nplikes. Minor thought - is it worth using a slightly shorter name like index instead of index_nplike? It would make the code slightly more readable, and I think it's safe enough given that there is no np.index.

@findingmochi findingmochi changed the title Refactoring to include index_nplike Refactoring to include index_nplike and reducers Jun 7, 2022
@jpivarski
Copy link
Member

I'm a +1 on this as well, it will be good to formally distinguish between index and content nplikes. Minor thought - is it worth using a slightly shorter name like index instead of index_nplike? It would make the code slightly more readable, and I think it's safe enough given that there is no np.index.

We could, although I think that would be easy to do at any time; nplikes are rather internal. Perhaps the nplike module ought to be made private (renamed to _nplike) so to formally say that we have this flexibility.

At this time, though, @swishdiff needs to finish his project (it's the last week!) and we can consider renaming later.

"index" might be a little understated, though. I would have trouble guessing what it means. Something like "for_index" would be more helpful, but that's getting back to the original length.

@agoose77
Copy link
Collaborator

agoose77 commented Jun 7, 2022

"index" might be a little understated, though. I would have trouble guessing what it means. Something like "for_index" would be more helpful, but that's getting back to the original length.

Actually, it's not just a length issue (which, in reality, I don't tend to weight that heavily). I should not have said "shorter", as it implied that length is an important metric.

Rather, I find nplike.index_nplike less readable for repetition than nplike.index.zeros or nplike.for_index.zeros.

We could, although I think that would be easy to do at any time; nplikes are rather internal. Perhaps the nplike module ought to be made private (renamed to _nplike) so to formally say that we have this flexibility.

OK, no problem - it won't take long to do should we see fit at a later date. Good luck @swishdiff!

@jpivarski
Copy link
Member

jpivarski commented Jun 8, 2022

JAX segment_prod has some subtleties: jax-ml/jax#9296 We're using the log → sum → exp that they recommend there.

@findingmochi
Copy link
Collaborator Author

@jpivarski please merge this when the tests pass. There are two cases in _slicing.py, that probably need special treatment for Jax, for now they raise an error if the nplike is Jax

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I approve! You can squash-and-merge.

@jpivarski jpivarski merged commit 70e4a99 into main Jun 8, 2022
@jpivarski jpivarski deleted the swishdiff/mixed-nplike branch June 8, 2022 19:07
@findingmochi findingmochi restored the swishdiff/mixed-nplike branch June 9, 2022 09:06
@jpivarski jpivarski deleted the swishdiff/mixed-nplike branch September 23, 2022 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants