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

Backward compatible with MXNet indexing. #1802

Merged
merged 10 commits into from
Jul 18, 2022

Conversation

KexinFeng
Copy link
Contributor

@KexinFeng KexinFeng commented Jul 16, 2022

This solves the bug reported in issue.

The new feature of indexing is not completely backward compatible, since NDIndexPick has been replaced by NDIndexTake. Now this problem is solved by treating NDIndexTake same as NDIndexPick in FullPick and then used in
api/src/main/java/ai/djl/ndarray/index/NDArrayIndexer.java line53. . Still, when receiving NDArray index, NDIndex will send it to NDIndexTake as the default defination for NDArray index. It is only muted to NDIndexPick on MXNet engine.

However, this is a temporary solution, only to make it backward compatible. NDIndexPick does not have the same definition as NDIndexTake when used to index array elements. For example, NDIndexPick cannot support larger-than-1-rank indexing array, while NDIndexTake can. So, as an NDIndexElement, NDIndexPick should be deprecated in the future.

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2022

Codecov Report

Merging #1802 (66fda19) into master (bb5073f) will decrease coverage by 2.52%.
The diff coverage is 65.08%.

@@             Coverage Diff              @@
##             master    #1802      +/-   ##
============================================
- Coverage     72.08%   69.56%   -2.53%     
- Complexity     5126     5494     +368     
============================================
  Files           473      524      +51     
  Lines         21970    24336    +2366     
  Branches       2351     2651     +300     
============================================
+ Hits          15838    16930    +1092     
- Misses         4925     6091    +1166     
- Partials       1207     1315     +108     
Impacted Files Coverage Δ
api/src/main/java/ai/djl/modality/cv/Image.java 69.23% <ø> (-4.11%) ⬇️
...rc/main/java/ai/djl/modality/cv/MultiBoxPrior.java 76.00% <ø> (ø)
...rc/main/java/ai/djl/modality/cv/output/Joints.java 71.42% <ø> (ø)
.../main/java/ai/djl/modality/cv/output/Landmark.java 100.00% <ø> (ø)
...main/java/ai/djl/modality/cv/output/Rectangle.java 72.41% <0.00%> (ø)
...i/djl/modality/cv/translator/BigGANTranslator.java 21.42% <ø> (-5.24%) ⬇️
...odality/cv/translator/BigGANTranslatorFactory.java 33.33% <0.00%> (+8.33%) ⬆️
...nslator/InstanceSegmentationTranslatorFactory.java 14.28% <0.00%> (-3.90%) ⬇️
.../cv/translator/SemanticSegmentationTranslator.java 0.00% <0.00%> (ø)
.../cv/translator/StyleTransferTranslatorFactory.java 40.00% <ø> (ø)
... and 415 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bc3689...66fda19. Read the comment docs.

@KexinFeng KexinFeng requested a review from frankfliu July 17, 2022 01:13
@lanking520 lanking520 merged commit 50a0563 into deepjavalibrary:master Jul 18, 2022
KexinFeng added a commit to KexinFeng/djl that referenced this pull request Jul 22, 2022
* backward compatible

* backward compatible

* error msg

* Update api/src/main/java/ai/djl/ndarray/types/Shape.java

Co-authored-by: Frank Liu <[email protected]>

* Update api/src/main/java/ai/djl/ndarray/types/Shape.java

Co-authored-by: Frank Liu <[email protected]>

* Update api/src/main/java/ai/djl/ndarray/types/Shape.java

Co-authored-by: Frank Liu <[email protected]>

* cast type

* cast type

* Fix issue for unsupported operator

* Reformat

Co-authored-by: Frank Liu <[email protected]>
@KexinFeng KexinFeng deleted the bug_fix2 branch August 25, 2022 00:15
@KexinFeng KexinFeng mentioned this pull request Aug 25, 2022
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.

4 participants