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

Support gamma objective in LGBMRegressor #484

Merged

Conversation

janjagusch
Copy link
Contributor

@janjagusch janjagusch commented Jul 27, 2021

With this PR, I'm adding support for the gamma objective for the LGBMRegressor.

I don't think the failing Windows tests are related to my changes.

@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from 0e45568 to d0d9f8b Compare July 27, 2021 15:47
@xadupre
Copy link
Collaborator

xadupre commented Jul 28, 2021

I'll look into the failure. Could you add a unit test to check the conversion works with this objective?

@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from 24ad09c to 4ca02fd Compare July 30, 2021 11:01
@janjagusch
Copy link
Contributor Author

janjagusch commented Jul 30, 2021

I'll look into the failure. Could you add a unit test to check the conversion works with this objective?

@xadupre, I have added test cases for all objectives that we're currently supporting. It's also easy to add more test cases for more objectives (e.g. tweedie, which I might add in a future PR). Hope the test suite is okay for you, usually I use pytest so I'm not very used to unittest.

@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch 2 times, most recently from c45270d to 9fccb8b Compare August 2, 2021 07:56
regressor_onnx: ModelProto = convert_lightgbm(regressor, initial_types=self._calc_initial_types(_X))
y_pred = regressor.predict(_X)
y_pred_onnx = self._predict_with_onnx(regressor_onnx, _X)
np.testing.assert_almost_equal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is failing for some of the configuration. It should pass with lower decimal. Otherwise, it looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the number of decimals won't make this test less flaky unfortunately (because of the precision difference in the split rules, the prediction error could be arbitrarily large). I added a new test function that allows 1 in 10,000 rows to have a difference larger than the specified decimals.

@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from 8dc9635 to 73c1f60 Compare August 3, 2021 06:07
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
…ws to be almost equal

Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from 2a7e924 to 041beee Compare August 3, 2021 07:09
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from 5d51dc0 to 51988cb Compare August 3, 2021 15:05
@janjagusch
Copy link
Contributor Author

@xadupre i think all tests that i can influence are succeeding now.

Two tests are failing that I don't think are related to my changes:

  • This is a different test case:
tests/coreml/test_cml_GLMClassifierConverter.py::TestCoreMLGLMClassifierConverter::test_glm_classifier
  /home/vsts/work/1/s/onnxmltools/utils/tests_helper.py:181: UserWarning: Issue with 'CmlBinLinearSVC-NoProb' due to Model 'tests/temp/CmlBinLinearSVC-NoProb.model.onnx' has discrepencies.
  <class 'onnxmltools.utils.utils_backend.OnnxRuntimeAssertionError'>: Unexpected output in model 'tests/temp/CmlBinLinearSVC-NoProb.model.onnx'
  max diff(expected, output)=3.691214992322873
  
  Arrays are not almost equal to 5 decimals
  
  Mismatched elements: 150 / 150 (100%)
  Max absolute difference: 3.69121499
  Max relative difference: 34.80315894
   x: array([-1.31647, -0.49216, -1.24896, -1.17561, -1.69487, -1.76238,
         -1.85322, -1.24312, -1.02892, -0.71803, -1.31064, -1.54817,
         -0.64469, -1.40731, -1.37815, -2.43416, -1.76238, -1.31647,...
   y: array([0.21141, 0.37938, 0.22288, 0.23584, 0.15514, 0.14649, 0.13549,
         0.22389, 0.26329, 0.32783, 0.21238, 0.17535, 0.34419, 0.19666,
         0.20131, 0.0806 , 0.14649, 0.21141, 0.25371, 0.11983, 0.34683,...
    warnings.warn("Issue with '{0}' due to {1}".format(basename, e))
  • And then one job is failing because the LgbmRegressor:TreeEnsembleRegressor is not implemented in the runtime.
onnxruntime.capi.onnxruntime_pybind11_state.NotImplemented: [ONNXRuntimeError] : 9 : NOT_IMPLEMENTED : Could not find an implementation for the node LgbmRegressor:TreeEnsembleRegressor(1)

Feel free to merge or let me know if you need anything else. 🙂

@xadupre
Copy link
Collaborator

xadupre commented Aug 4, 2021

Thanks, I'll have a look. The broken tests are not related. I noticed some weird failure too (coremltools 3.1 fails with scikit-learn 0.24.2 on Windows and python 3.9).

@janjagusch
Copy link
Contributor Author

Thanks, I'll have a look. The broken tests are not related. I noticed some weird failure too (coremltools 3.1 fails with scikit-learn 0.24.2 on Windows and python 3.9).

@xadupre, considering that the failures are not related to the changes, do you think we could merge this? 🙂

@janjagusch
Copy link
Contributor Author

Thanks, I'll have a look. The broken tests are not related. I noticed some weird failure too (coremltools 3.1 fails with scikit-learn 0.24.2 on Windows and python 3.9).

@xadupre, considering that the failures are not related to the changes, do you think we could merge this? 🙂

ping, @xadupre. 🙂

@xadupre
Copy link
Collaborator

xadupre commented Aug 11, 2021

It is possible to fix CI? The failing test fails because it is run with an old version of onnxruntime (1.0). Can you disable the test when onnxruntime is older than 1.3?

Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from e8ef764 to 53712e6 Compare August 12, 2021 06:33
Signed-off-by: Jan-Benedikt Jagusch <jan.jagusch@gmail.com>
@janjagusch janjagusch force-pushed the support-gamma-objective-in-lgbm-regressor branch from cdc12ab to aa6c525 Compare August 12, 2021 08:10
@janjagusch
Copy link
Contributor Author

It is possible to fix CI? The failing test fails because it is run with an old version of onnxruntime (1.0). Can you disable the test when onnxruntime is older than 1.3?

Fixed the CI, ready to merge. 🙂

@xadupre xadupre merged commit b5dc2f6 into onnx:master Aug 12, 2021
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.

2 participants