Skip to content

[c++] Preserve feature sampling on unrelated parameter updates - #7458

Open
xiaoh1024 wants to merge 3 commits into
lightgbm-org:mainfrom
xiaoh1024:fix/feature-sampling-reset
Open

xiaoh1024 wants to merge 3 commits into
lightgbm-org:mainfrom
xiaoh1024:fix/feature-sampling-reset

Conversation

@xiaoh1024

Copy link
Copy Markdown
Contributor

Updating unrelated parameters triggers extra feature sampling, causing equivalent built-in and custom objectives to select different features. Skip resampling in ColSampler::SetConfig() when feature_fraction and feature_fraction_seed are unchanged.

Fixes #6053.

I used an AI agent to help with the analysis, code, tests, and PR description. I reviewed the code and description.

@jameslamb jameslamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. The fix makes sense but I'd like to see the tests made much stricter, and I've asked a clarifying question about one of them.

Please see my recommendations in #7459 (comment) for some ideas about what I mean by "much stricter".

Comment on lines +3159 to +3160
"max_depth": 1,
"learning_rate": 0.001,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"max_depth": 1,
"learning_rate": 0.001,

I don't see a reason to override the default for learning_rate here, let's remove this.

And I think if you follow my other recommendations, you'll need to train deeper trees than max_depth: 1.

def custom_l2(pred, data):
return pred - data.get_label(), np.ones_like(pred)

def reset_same_learning_rate(env):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry but I am not understanding this... how is the learning rate involved in any of this? I don't understand what the reset_parameter tests are covering here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the goal is just to set some parameter to a non-default value to be sure the initial SetConfig() call(s) do not exit early, that makes sense to me... but the tests aren't ever checking that learning_rate = 0.001 made it through into the Booster.

Either way, please do add some assertions here checking that other overrides (learning_rate is fine) do make it through into the Booster on the C++ side. The test code could call Booster._get_loaded_param() for help with that:

def _get_loaded_param(self) -> Dict[str, Any]:
buffer_len = 1 << 20
tmp_out_len = ctypes.c_int64(0)
string_buffer = ctypes.create_string_buffer(buffer_len)
ptr_string_buffer = ctypes.c_char_p(ctypes.addressof(string_buffer))
_safe_call(
_LIB.LGBM_BoosterGetLoadedParam(
self._handle,
ctypes.c_int64(buffer_len),
ctypes.byref(tmp_out_len),
ptr_string_buffer,
)
)

@xiaoh1024

Copy link
Copy Markdown
Contributor Author

@jameslamb Thanks for your suggestions! reset_parameter was intended to check that resetting an unrelated parameter does not advance the feature-sampling state. I originally reset the learning rate to its existing value to keep training equivalent to the reference, making differences caused by extra sampling detectable. I have also adopted your other suggestions, just like #7459.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature fraction gives different column selections when using a custom objective

2 participants