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

Use PyTorch index to improve the performance #638

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

stu1130
Copy link
Contributor

@stu1130 stu1130 commented Feb 10, 2021

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@@ -265,6 +265,12 @@ public void set(Buffer data) {
JniUtils.set(this, buf);
}

/** {@inheritDoc} */
Copy link
Contributor

Choose a reason for hiding this comment

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

If we put it here, it may not activate for some calls to get that use the NDIndex directly. The more precise place would be to integrate it into the NDArrayIndexer. Maybe add a new NDIndexFull type to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked it but can't come up with a good way to avoid NDIndexFullPick.fromIndex and NDIndexFullSlice.fromIndex. Maybe we can merge it for now and later optimize it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should avoid fullpick on its own because pick requires a pick NDIndexElement, while this is a fixed. Avoiding the slice is difficult, but there is an order of trying the NDIndexFull* inside the NDArrayIndexer. Just make your new one before NDIndexFullSlice in the order. In fact, maybe just make your new one first in the order. You may also need to throw an exception in the FullFixed get and catch it in the NDArrayIndexer get to make the FullFixed handling optional.

If you prefer, go ahead and merge it and then I can take a look at it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok Thank, I will go ahead to merge it and you can come back to this when you have time

@stu1130 stu1130 merged commit 3bed9fa into deepjavalibrary:master Feb 11, 2021
@stu1130 stu1130 deleted the getitem branch February 11, 2021 00:11
Lokiiiiii pushed a commit to Lokiiiiii/djl that referenced this pull request Oct 10, 2023
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