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

[FEA] Raise a warning if LightGBM 64-bit thresholds may cause rounding issues #2817

Closed
JohnZed opened this issue Sep 11, 2020 · 3 comments · Fixed by #3316
Closed

[FEA] Raise a warning if LightGBM 64-bit thresholds may cause rounding issues #2817

JohnZed opened this issue Sep 11, 2020 · 3 comments · Fixed by #3316
Labels
feature request New feature or request

Comments

@JohnZed
Copy link
Contributor

JohnZed commented Sep 11, 2020

Once treelite supports float64, FIL can detect this and issue a warning.

@JohnZed JohnZed added feature request New feature or request ? - Needs Triage Need team to review and classify labels Sep 11, 2020
@wphicks wphicks removed the ? - Needs Triage Need team to review and classify label Sep 14, 2020
@JohnZed
Copy link
Contributor Author

JohnZed commented Sep 24, 2020

Could possibly do this in the cuML conversion process (before getting into treelite)

@hcho3
Copy link
Contributor

hcho3 commented Sep 24, 2020

We can add a warning about float64 here:

def from_filename(filename, model_type="xgboost"):
"""
Returns a TreeliteModel object loaded from `filename`
Parameters
----------
filename : string
Path to treelite model file to load
model_type : string
Type of model: 'xgboost', or 'lightgbm'
"""
filename_bytes = filename.encode("UTF-8")
cdef ModelHandle handle
if model_type == "xgboost":
res = TreeliteLoadXGBoostModel(filename_bytes, &handle)
if res < 0:
err = TreeliteGetLastError()
raise RuntimeError("Failed to load %s (%s)" % (filename, err))
elif model_type == "lightgbm":
res = TreeliteLoadLightGBMModel(filename_bytes, &handle)
if res < 0:
err = TreeliteGetLastError()
raise RuntimeError("Failed to load %s (%s)" % (filename, err))
else:
raise ValueError("Unknown model type %s" % model_type)
model = TreeliteModel()
model.set_handle(handle)
return model

This covers ForestInference.load() but does not yet cover ForestInference.load_from_treelite_model() or ForestInference.load_using_treelite_handle(). To cover these two, we'd need to bring in the Treelite refactor (dmlc/treelite#196).

@hcho3
Copy link
Contributor

hcho3 commented Dec 30, 2020

#3316 is needed to reliably detect all Treelite models with float64 values.

@wphicks wphicks removed their assignment Dec 31, 2020
@rapids-bot rapids-bot bot closed this as completed in #3316 Jan 7, 2021
rapids-bot bot pushed a commit that referenced this issue Jan 7, 2021
Depends on dmlc/treelite#235, dmlc/treelite#236, dmlc/treelite#237, dmlc/treelite#240, dmlc/treelite#241, dmlc/treelite#242, dmlc/treelite#243

Depends on rapidsai/integration#201

Closes #2160
Closes #2795
Closes #2817

TODOs
- [x] Get code review for dmlc/treelite#235 and dmlc/treelite#236 and merge them
- [x]  Get code review for dmlc/treelite#237 and merge them
- [x] Merge dmlc/treelite#240, dmlc/treelite#241, dmlc/treelite#242, dmlc/treelite#243
- [x] Run tests locally
- [x] Tag a new version of Treelite
- [x] Release a new Conda package with the new version tag
- [x] Update `meta.yml` and the integration package to use the new Treelite
- [x] Run tests (this time build with new Conda package)

Authors:
  - Hyunsu Cho <[email protected]>

Approvers:
  - AJ Schmidt (@ajschmidt8)
  - William Hicks (@wphicks)
  - John Zedlewski (@JohnZed)

URL: #3316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants