Repository navigation
Fix NaN ordering in GFR feature presorting - #427
Merged
andrewherren merged 3 commits intoOct 5, 2026
Merged
Conversation
andrewherren
added a commit
to MichaelChirico/stochtree
that referenced
this pull request
Oct 5, 2026
Add Bug Fixes entries for StochasticTree#425, StochasticTree#426, StochasticTree#427, and StochasticTree#428, noting that they change sampler output for a given seed, and add David Wachsmuth as a contributor in DESCRIPTION and the package Rd. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Sorry, here's one more. All came from investigating the same crashes, and with these three fixes on my local fork, my project is now running successfully!
FeaturePresortRoot::ArgsortRoot()currently passes ordinary floating-point less-than tostd::stable_sort. With unequal non-NaN values and NaNs, this does not provide a strict weak ordering:0and NaN compare equivalent, and NaN and1compare equivalent, but0and1do not. Presorted rows can consequently leave NaNs interleaved with nonmissing values or leave nonmissing values out of order. GFR relies on these sorted rows when scoring candidate splits, while numeric partitioning routes NaNs right becauseNaN <= thresholdis false. A malformed sorted prefix can therefore disagree with actual routing.This change sorts non-NaNs ascending and NaNs last, retaining stable order for equal values and for NaNs. It preserves the existing comparator behavior for inputs without NaNs, including infinities and signed-zero ties. It does not introduce a learned missing direction or a new missing-data modeling strategy. The change is independent of the MCMC extrema and acceptance fixes.
Three C++ regression tests cover all 720 index permutations of six observations with two NaNs and a finite tie, agreement between the sorted prefix and numeric partitioning, nonmissing order/stability, and empty/singleton/all-NaN inputs. The mixed-data test fails against the unmodified upstream header and passes with this change. The full C++ suite passes all 31 tests. A standalone six-observation C++ reproducer exercising the actual presort is included in the accompanying submission materials.
The issue was discovered while investigating native crashes during GFR on numeric predictors with missing values. In separate R-0.4.5 diagnostics, this comparator repair alone eliminated two previously reproduced crashes; patched AddressSanitizer checks also passed. The small regression test targets the ordering defect rather than promising a portable native crash reproducer. Missing-data fits can change even at the same seed.