Repository navigation
[ML] Skip missing values in fixed candidate splits - #3240
Open
HaohanTsao wants to merge 1 commit into
Open
HaohanTsao wants to merge 1 commit into
HaohanTsao wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3241
This PR goes with elastic/elasticsearch#161539. Please merge that PR first, or at the same time as this one. If this PR is merged first, analytics jobs will give normal feature importance for the affected models, but inference in Elasticsearch will still give NaN.
The problem
If a feature has only a few distinct values and some missing values, boosted tree training can build a broken model, and the job does not report any error.
CBoostedTreeImpl::initializeFixedCandidateSplitscollects the distinct values of each feature to build the candidate splits. It did not skip missing values (NaN). NaN is never equal to itself, so every missing row was added as a new value. If a feature still had 75 values or fewer, it used the fixed-candidate path. There, NaN brokestd::sort, and some candidate splits became NaN.Changes
In
initializeFixedCandidateSplits:reserve(values.size() - 1)throwsstd::length_error. This can happen when the encoding was computed on a different data set.refreshSplitsCachealready does this.In
CTreeShapFeatureImportance: handle nodes with no samples. Models trained before this fix still have these nodes. Incremental training can also create them in a normal way, because it counts the samples of old trees again on new data. I reproduced a model with 1,184 such nodes.shapRecursive, if a node below the root has no samples, its two children each get half of the fraction.computeInternalNodeValues, such a node gets the simple average of its two children.TreeInferenceModel) in [ML] Fix NaN feature importance for trees with zero-sample nodes elasticsearch#161539.incoming * (child / node), in the same order as the Java code. The existing SHAP tests pass with no changes, at their 1e-7 tolerance.computeInternalNodeValuesexplains these cases.Testing
New unit tests. I ran each test first on code where it should fail, and checked that it failed for the expected reason.
CBoostedTreeTesttestLowCardinalityFeaturesFixedCandidateSplitsWithMissingValuestestLowCardinalityFeaturesWithMissingValuestestLowCardinalityFeatureAllMissingAfterSeparateEncodingstd::length_error. This test also passes on the original code.CTreeShapFeatureImportanceTesttestZeroSampleInnerNodeExpectedNodeValuestestZeroSampleInnerNodeShaptestZeroSampleRootIsNotMaskedtestInconsistentSampleCountsAreNotMaskedTest suites (local run, macOS arm64, Apple clang 17, Boost 1.86):
ml_test_maths_analytics: 126/127 passed, 1 skipped. The skipped test istestMultinomialLogisticRegressionForManyClasses, which is disabled. Before the change: 119/120, with the same skip.ml_test_api: 225/226 passed.CFieldDataCategorizerTest/testJobKilledReverseSearchfails in the same way on the original code in my environment, so it is not related.ml-check-style:2image) shows no changes.End to end: Elasticsearch
mainwith my local build ofdata_frame_analyzer. The builds before and after the fix use the same toolchain.At 20%, the standard error is about 0.006, so this difference is just noise.
n_gram_encodingclassification on domain names: zero-sample splits went from 1.2%, 13.3% and 34.3% down to 0% for three seeds. Native and Java feature importance match within 3e-15.