Skip to content

Fix extrema calculation in MCMC split-range and node-constant helpers - #425

Open
dwachsmuth wants to merge 1 commit into
StochasticTree:mainfrom
dwachsmuth:fix-mcmc-extrema
Open

Fix extrema calculation in MCMC split-range and node-constant helpers#425
dwachsmuth wants to merge 1 commit into
StochasticTree:mainfrom
dwachsmuth:fix-mcmc-extrema

Conversation

@dwachsmuth

Copy link
Copy Markdown

Hello,

I bumped into this issue in the course of a modelling project involving many thousands of model fits. I had GPT 6 Astra get to the bottom of the problem; it found the issue and proposed the solution, and I've verified that it works, at least on my local machine.

Here's the issue, solution and reprex:

VarSplitRange() can return an inverted interval for a varying numeric feature: observations visited in order 3, 2, 1 produce [1, DBL_MIN] instead of [1, 3]. MCMCGrowTreeOneIter() then returns without proposing a split because the upper bound is below the lower bound. Negative features can instead get an upper bound near zero even when all observations are strictly negative.

There are two causes: numeric_limits<double>::min() is the smallest positive normalized double, and the if/else if extrema update prevents an observation from updating both bounds. The latter also makes results depend on iteration order. NodeNonConstant() and NodesNonConstantAfterSplit() use the same patterns and can misclassify varying positive or constant nonpositive features.

This patch initializes maxima with lowest() and updates both extrema independently in all three helpers. It preserves their signatures and existing control flow, including the child-constant combination logic. Three deterministic GoogleTest cases exercise ranges and constant checks over permutations of small positive, negative, mixed-sign, constant and singleton inputs. They call the real helpers with ForestDataset and ForestTracker; no stochastic model fit is needed.

Minimal reproduction (also covered by SamplerExtrema.RangeIsIndependentOfOrderAndSign):

std::vector<double> x{3, 2, 1};
StochTree::ForestDataset data;
data.AddCovariates(x.data(), 3, 1, true);
std::vector<StochTree::FeatureType> types{StochTree::FeatureType::kNumeric};
StochTree::ForestTracker tracker(data.GetCovariates(), types, 1, 3);
double lo, hi;
StochTree::VarSplitRange(tracker, data, 0, 0, 0, lo, hi);
// Before: lo == 1, hi == std::numeric_limits<double>::min()
// After:  lo == 1, hi == 3

Validation on macOS ARM64 with Apple Clang, C++17 and OpenMP disabled:

  • Before the fix, all three new regression tests fail.
  • After the fix, all 31 C++ tests pass, including the three new tests.
  • The standalone reproducer changes from [1, 2.2250738585072014e-308]
    (exit 1) to [1, 3] (exit 0).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant