[RF] Default RooFit variables to zero bins instead of 100#22705
Open
guitargeek wants to merge 2 commits into
Open
[RF] Default RooFit variables to zero bins instead of 100#22705guitargeek wants to merge 2 commits into
guitargeek wants to merge 2 commits into
Conversation
weightInterpolated() took the y-dimension binning from the caller's variable instead of the one owned by the RooDataHist. These can differ (e.g. RooHistPdf passes its own observable clones), and since the bin indexing relies on _idxMult and centralIdx (expressed in the internal binning), a mismatch yields out-of-range bins and a wrong (often zero) weight. Use the internal binning, like calcTreeIndex() and interpolateDim() already do.
A freshly-constructed RooRealVar (and RooErrorVar) used to have a
default binning of 100 bins. Since 100 is also a valid user choice,
there was no way to tell whether a variable's bin count was deliberately
set or just left at the default. This caused redundant "nbins" fields to
be written when serializing workspaces to HS3 JSON.
RooAbsRealLValue::getBins() now returns 0 until a binning is explicitly
set, so HS3 can write "nbins" only when getBins() != 0 (previously the
heuristic was getBins() != 100, which both missed a deliberate 100 and
churned on the default).
The historical default is re-injected, via the new constant
RooAbsRealLValue::DefaultNBins (100), wherever code legitimately relied
on it:
- unbinned-dataset plotting (RooAbsRealLValue::frame and overloads)
- generateBinned / direct RooDataHist construction (materialized on
the dataset's own variable clone in RooDataHist::initialize, so the
user's variable is untouched)
- createHistogram (RooAbsRealLValue and RooAbsData)
- the RooHist "P" draw-option path
So plotting, generating binned data and creating histograms from a
default-constructed variable behave exactly as before.
Also fixes a latent bug where RooUniformBinning's default constructor
left its members uninitialized, and guards RooUniformBinning::setRange
against division by zero for a zero-bin binning. ROOT I/O is unaffected:
binnings are restored from the persisted object, so only
newly-constructed variables default to zero bins.
🤖 Done with the help of AI.
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.
A freshly-constructed RooRealVar (and RooErrorVar) used to have a default binning of 100 bins. Since 100 is also a valid user choice, there was no way to tell whether a variable's bin count was deliberately set or just left at the default. This caused redundant "nbins" fields to be written when serializing workspaces to HS3 JSON.
RooAbsRealLValue::getBins() now returns 0 until a binning is explicitly set, so HS3 can write "nbins" only when getBins() != 0 (previously the heuristic was getBins() != 100, which both missed a deliberate 100 and churned on the default).
The historical default is re-injected, via the new constant RooAbsRealLValue::DefaultNBins (100), wherever code legitimately relied on it:
So plotting, generating binned data and creating histograms from a default-constructed variable behave exactly as before.
Also fixes a latent bug where RooUniformBinning's default constructor left its members uninitialized, and guards RooUniformBinning::setRange against division by zero for a zero-bin binning. ROOT I/O is unaffected: binnings are restored from the persisted object, so only newly-constructed variables default to zero bins.
🤖 Done with the help of AI.
FYI, @Phmonski, who raised the point that it's not possible to detect whether nbins == 100 was just the default or explicitly set by the user, which is a problem for HS3 serialization.