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

[LightGBM] Create initial LightGBM engine #1895

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

zachgk
Copy link
Contributor

@zachgk zachgk commented Aug 11, 2022

No description provided.

@zachgk zachgk requested a review from frankfliu as a code owner August 11, 2022 23:19
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022

Codecov Report

Merging #1895 (4c8235c) into master (bb5073f) will decrease coverage by 2.31%.
The diff coverage is 66.71%.

@@             Coverage Diff              @@
##             master    #1895      +/-   ##
============================================
- Coverage     72.08%   69.77%   -2.32%     
- Complexity     5126     5643     +517     
============================================
  Files           473      539      +66     
  Lines         21970    24957    +2987     
  Branches       2351     2706     +355     
============================================
+ Hits          15838    17413    +1575     
- Misses         4925     6209    +1284     
- Partials       1207     1335     +128     
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% <0.00%> (-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 437 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

/** {@code LgbmNDArray} is the LightGBM implementation of {@link NDArray}. */
public class LgbmNDArray extends NDArrayAdapter {

private ByteBuffer data;
Copy link
Contributor

Choose a reason for hiding this comment

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

This part looks strange to me. Why we are trying to keep a ByteBuffer reference instead of storing a native pointer for the data? And I can see you are trying to pass the data to native only during getHandle() function. Shoudn't we just create the handle and pass it to the NDArray?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need keep both handler and ByteBuffer.

The problem the handle is write only, cannot read data from it. It can only used for inference.

So we have to have two type of NDArray:

  1. input only NDArraty, we create handle and pass to engine for inference
  2. output only NDArray, we get bytes from the model and need return a NDArray, we don't have handle in this case.

/** {@code LgbmNDArray} is the LightGBM implementation of {@link NDArray}. */
public class LgbmNDArray extends NDArrayAdapter {

private ByteBuffer data;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need keep both handler and ByteBuffer.

The problem the handle is write only, cannot read data from it. It can only used for inference.

So we have to have two type of NDArray:

  1. input only NDArraty, we create handle and pass to engine for inference
  2. output only NDArray, we get bytes from the model and need return a NDArray, we don't have handle in this case.

engines/ml/lightgbm/build.gradle Outdated Show resolved Hide resolved
zachgk and others added 2 commits August 12, 2022 10:09
Change-Id: I67533d25c61d26d9e24b5bc70b0993569b483cc8
@zachgk zachgk merged commit 39f59e1 into deepjavalibrary:master Aug 12, 2022
@zachgk zachgk deleted the lightgbm branch August 12, 2022 19:34
zachgk added a commit to zachgk/djl that referenced this pull request Aug 15, 2022
…1895)

* [LightGBM] Create initial LightGBM engine

* Fix copyright years and build.gradle exclusions

* minor update for unit test

Change-Id: I67533d25c61d26d9e24b5bc70b0993569b483cc8

Co-authored-by: Frank Liu <[email protected]>
patins1 pushed a commit to patins1/djl that referenced this pull request Aug 26, 2022
* [LightGBM] Create initial LightGBM engine

* Fix copyright years and build.gradle exclusions

* minor update for unit test

Change-Id: I67533d25c61d26d9e24b5bc70b0993569b483cc8

Co-authored-by: Frank Liu <[email protected]>
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