search

IQVIA-ML / LightGBM.jl

Julia FFI interface to Microsoft's LightGBM package
Other
93 stars 10 forks source link

fix: prevent mutation of categorical_feature inside stringifyparams #133

Closed characat0 closed 4 months ago

characat0 commented 1 year ago

Fixes https://github.com/IQVIA-ML/LightGBM.jl/issues/131

Assign to a new variable instead of assigning inplace. This is useful during CV where the model is trained many times.

This is a copy of https://github.com/IQVIA-ML/LightGBM.jl/pull/132 but from a cleaner branch

kainkad commented 1 year ago

HI @characat0 thank you for raising the issue and submitting PR. I have left comments on your PR as it currently doesn't seem to address the issue raised. Are you ok to follow up on this?

characat0 commented 1 year ago

Thanks for the feedback, @kainkad. Will submit these changes.

characat0 commented 4 months ago

We missed a config for the test LGBM_BoosterUpdateOneIterCustom, in https://github.com/microsoft/LightGBM/pull/5438 they added an explicit test for objective==nullptr

kainkad commented 4 months ago

We missed a config for the test LGBM_BoosterUpdateOneIterCustom, in microsoft/LightGBM#5438 they added an explicit test for objective==nullptr

Thanks for fixing the mutated indices - it does work fine after the fix so happy to merge. I have a question on the null pointer, isn't it a default for the objective unless specified though?

characat0 commented 4 months ago

I have a question on the null pointer, isn't it a default for the objective unless specified though?

I thought that too, but looks like the default objective is 'regression', I've confirmed this using LightGBM.LGBM_BoosterSaveModelToString