include parameters from reference dataset on subset (fixes #5402) #5416
Conversation
jmoralez
requested review from
guolinke,
shiyu1994,
StrikerRUS and
jameslamb
as code owners
| @@ -243,6 +243,13 @@ def test_chunked_dataset_linear(): | |||
| valid_data.construct() | |||
|
|
|||
|
|
|||
| def test_save_dataset_subset_and_load_from_file(tmp_path): | |||
| data = np.random.rand(100, 2) | |||
| ds = lgb.Dataset(data) | |||
There was a problem hiding this comment.
Could you please try overriding the defaults (e.g. setting max_bin: 7 or something) and check that those non-default values survive the round trip to disk?
I think that would increase our confidence that this is working as expected. Otherwise, I think a bug of the form "all parameter values are lost when writing to binary Dataset file" could make it through this test.
There was a problem hiding this comment.
I think that will raise an error like the one in #4904, but I'll try it and confirm here.
There was a problem hiding this comment.
Ok I see. I expected a PR called "include parameters" to test that parameters on either side of an operation had the same values.
I think maybe I got confused by the presence of writing and reading a binary file, and thought the issue was specific to storing a Dataset to disk. Is it like "when taking a subset, not all parameters are copied from the reference Dataset to the subset...and this can show up as an error loading the Dataset from file"?
If it is, then it would be great to be able to reach into the Dataset (on the C++ side, not the Python object) and check that attributes like max_bin_ are the same and a non-default value before writing to a file. But I'm not sure how to do that without introducing a new c_api entrypoint.
So if adding the test I suggested does hit the error from #4904, then I think this test in its current state is ok. It is still an improvement that fixes a bug.
There was a problem hiding this comment.
I built the dataset with non default parameters and check that loading it with the same ones succeeds in 6a2fd1f.
There was a problem hiding this comment.
Ok nice! Thank you for that and for the explanation.
I tried running this PR's test code on latest master, and can see based on the error message that those non-default parameter values are being respected.
import lightgbm as lgb
import numpy as np
data = np.random.rand(100, 2)
params = {'max_bin': 50, 'min_data_in_bin': 10}
ds = lgb.Dataset(data, params=params).construct()
ds.subset([1, 2, 3, 5, 8]).save_binary('subset.bin')
lgb.Dataset('subset.bin', params=params).construct()[LightGBM] [Fatal] Dataset was constructed with parameter max_bin=32649. It cannot be changed to 50 when loading from binary file.
I'm not sure where that 32649 is coming from, but I think that's not an issue caused by this PR.
jmoralez
changed the title
There was a problem hiding this comment.
Thanks for this!
I ran the test code on latest master (865c126) and confirmed that this test fails with the error
[LightGBM] [Fatal] Dataset was constructed with parameter max_bin=32649. It cannot be changed to 50 when loading from binary file.
The fix here looks correct to me.
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Fixes #5402