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

Port LightGBM provider #26

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

shengwangsw
Copy link
Contributor

Port LightGBM provider from author Alberto Ferreira ([email protected]).

@codecov
Copy link

codecov bot commented Jul 3, 2020

Codecov Report

Merging #26 into master will increase coverage by 1.49%.
The diff coverage is 80.90%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #26      +/-   ##
============================================
+ Coverage     76.19%   77.69%   +1.49%     
- Complexity      268      357      +89     
============================================
  Files            28       39      +11     
  Lines           899     1318     +419     
  Branches         79      120      +41     
============================================
+ Hits            685     1024     +339     
- Misses          171      228      +57     
- Partials         43       66      +23     
Impacted Files Coverage Δ Complexity Δ
...i/openml/provider/lightgbm/LightGBMMLProvider.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...er/lightgbm/LightGBMBinaryClassificationModel.java 57.14% <57.14%> (ø) 8.00 <8.00> (?)
...feedzai/openml/provider/lightgbm/LightGBMSWIG.java 72.13% <72.13%> (ø) 11.00 <11.00> (?)
...eedzai/openml/provider/lightgbm/SWIGResources.java 73.33% <73.33%> (ø) 7.00 <7.00> (?)
...openml/provider/lightgbm/LightGBMModelCreator.java 82.35% <82.35%> (ø) 20.00 <20.00> (?)
...i/openml/provider/lightgbm/SWIGTrainResources.java 83.78% <83.78%> (ø) 9.00 <9.00> (?)
...eedzai/openml/provider/lightgbm/LightGBMUtils.java 86.95% <86.95%> (ø) 4.00 <4.00> (?)
...tgbm/LightGBMBinaryClassificationModelTrainer.java 87.28% <87.28%> (ø) 23.00 <23.00> (?)
...enml/provider/lightgbm/LightGBMDescriptorUtil.java 96.96% <96.96%> (ø) 3.00 <3.00> (?)
...i/openml/provider/lightgbm/LightGBMAlgorithms.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
... and 12 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 de052b7...60902db. Read the comment docs.

@paulojrp
Copy link
Contributor

paulojrp commented Jul 3, 2020

Rename the folder of the new provider from openml-lightgbm-provider to openml-lightgbm

@ptraca
Copy link

ptraca commented Jul 3, 2020

@shenggwang I've finished the review, please see comments.

openml-lightgbm-provider/lightgbmlib_build/pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/pom.xml Outdated Show resolved Hide resolved
openml-lightgbm-provider/lightgbmlib_build/__commit_id__ Outdated Show resolved Hide resolved
Copy link
Contributor

@AlbertoEAF AlbertoEAF left a comment

Choose a reason for hiding this comment

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

It looks good @shenggwang.

In fact great work with the additions of "final" and fixes in the docstrings.

The visibility modifier changes should be reverted though.
We were extremely careful and purposefully expose only what's needed. For instance, many internal-use-only classes and properties are now public and user-visible, which should not happen.

I have one question for @JPDSousa , by dropping those TODO's, it means we'll have to maintain 2 versions of the provider? The open source and the internal?

@AlbertoEAF
Copy link
Contributor

I have one final question, if we strip away the versions as above, then from now on how do we track the LightGBM provider version internally?

@shengwangsw
Copy link
Contributor Author

I have one final question, if we strip away the versions as above, then from now on how do we track the LightGBM provider version internally?

I suppose that from now on, we are going to track internally the version of LightGBM on openml-java.

@ptraca
Copy link

ptraca commented Jul 6, 2020

I've talked this over with @JPDSousa and we need to also move this repo [email protected]:data-science/make-lightgbm.git to github. Maybe we could add it in this repo as a git sub-module replacing the lightgbmlib_builddir.

@AlbertoEAF
Copy link
Contributor

I've talked this over with @JPDSousa and we need to also move this repo [email protected]:data-science/make-lightgbm.git to github. Maybe we could add it in this repo as a git sub-module replacing the lightgbmlib_builddir.

I'd rather keep it apart if possible @ptraca , the make-lightgbm generates a lot of trash and temporary files and stuff that is not needed here. Also, both repos are agnostic to each other, and make-lightgbm serves to build artifacts for any version of LightGBM and Java LightGBM implementation, not only ours.

@shengwangsw
Copy link
Contributor Author

@ptraca @JPDSousa @AlbertoEAF @paulojrp
I think this is done. Take a look.

Copy link
Contributor

@paulojrp paulojrp left a comment

Choose a reason for hiding this comment

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

lgtm

good work

@shengwangsw shengwangsw merged commit d91ddff into master Jul 10, 2020
@shengwangsw shengwangsw deleted the ft-sw-PULSEDEV-31197-add-openml-lightgbm-provider branch July 10, 2020 21:19
shengwangsw added a commit that referenced this pull request Aug 12, 2020
Port lightgbm provider
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.

5 participants