Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RF] Fitting RooSimultaneous with Range option not accounting for range on the indexCat #8231

Closed
will-cern opened this issue May 24, 2021 · 4 comments · Fixed by #11785
Closed

Comments

@will-cern
Copy link
Contributor

will-cern commented May 24, 2021

The expected behaviour here would be if I define a range on the indexCat, the fit should only involve the pdfs of the categories in the range if I specify Range("rangeName") as an option to the fitTo method.

Attached is a demo workspace containing a two-channel model with a floating norm factor on each channel. Ranges "r1" and "r2" have been defined on the category var which respectively only include one of the channels. However, running:

w->pdf("simPdf")->fitTo(*w->data("obsData"),RooFit::Range("r1"))

on the workspace shows that the floating parameter for both channels gets fit to.

simDemo.zip

@will-cern will-cern added the bug label May 24, 2021
@guitargeek guitargeek changed the title Fitting RooSimultaneous with Range option not accounting for range on the indexCat [RF] Fitting RooSimultaneous with Range option not accounting for range on the indexCat Jun 22, 2022
@guitargeek
Copy link
Contributor

Hi, thanks for opening this issue. Sorry that nobody was looking at this yet, I see that you already opened a Jira issue about this many years ago (I closed the Jira issue now as duplicate). Is this problem still relevant? I would start working on this then.

@guitargeek
Copy link
Contributor

Just for the paper trail, here is Will's original text from the 2016 Jira issue 8304:

Hello,

I am trying to run a fit in a restricted range where one of the observables is a RooCategory. I have defined a range that contains the states I want to consider in the fit. I am trying to do the fit with pdf->fitTo(data,Range("myrange")). I have noticed that in RooAbsOptTestStatistic that the reduce method is called on the input data, which will correctly restrict the fit to just the data in my desired fit range, including the RooCategory observable. But there is no restriction placed on the discrete observables, whereas there is one on the continuous ones: https://root.cern.ch/gitweb/?p=root.git;a=blob;f=roofit/roofitcore/src/RooAbsOptTestStatistic.cxx;hb=01602ce4a5f22fde41f2543cfaa066e0c9007714#l301

Could the code at this location be extended to support restricting the range of discrete observables in the fit?

Thanks
Will

@guitargeek
Copy link
Contributor

I'll also change the tag from "bug" to "feature request". It's not obvious that a discrete category can have a "range", because a range is over something that is ordered. A RooCategory represents discrete, unordered labels, so I'd say the "range" doesn't apply to them. What is the usecase for this range support? Is it for fitting only some channels in a RooSimultaneous? Maybe we should find another way to do this easily, some way that also makes conceptually more sense than exploiting the range mechanism.

What do you think?

@will-cern
Copy link
Contributor Author

So I think it's well defined what we mean by the range of a discrete variable - each value is declared as either being in the range or not in the range. RooFit already supports this for RooCategory, so that's all well and good. I do think it makes sense then to think about the behaviour of fits when such a range is declared. It's actually possible my original jira issue might have been referring to a case where I had a PDF that wasn't a RooSimultaneous but was dependent on a RooCategory and it wasn't handled correctly, I'd have to look back at it and remind myself. But for sure in the case of the RooCategory being the index category of a RooSimultaneous I think it makes total sense for the RooSimultaneous to be 'reduced' to just the channels that are in the range.

guitargeek added a commit to guitargeek/root that referenced this issue Nov 27, 2022
A new unit test is implemented that verifies the `RooFit::Range()`
command argument for fitTo can be used to select specific components
from a RooSimultaneous.

Covers GitHub issue root-project#8231.
guitargeek added a commit to guitargeek/root that referenced this issue Nov 27, 2022
guitargeek added a commit to guitargeek/root that referenced this issue Nov 27, 2022
guitargeek added a commit to guitargeek/root that referenced this issue Nov 27, 2022
A new unit test is implemented that verifies the `RooFit::Range()`
command argument for fitTo can be used to select specific components
from a RooSimultaneous.

Covers GitHub issue root-project#8231.
guitargeek added a commit to guitargeek/root that referenced this issue Nov 28, 2022
It is possible to set ranges for a `RooCategory`, suggesting that this
is a possible way to select only a subset of channels in a simultaneous
fit. However, this was not working so far, and this commit implements
that.

This commit implements that feature for both the old test statistic
classes and the new BatchMode.

As now it also makes sense to fit to a range that is not defined for the
observables but for the categories only, the message that is printed
when your observables don't define the range is demoted from a warning
to an info message.

The debug message that was printed when channels are not selected also
got removed, because it had some overhead from `sumEntries`, the
debugging prints are rarely used, and the message is not true anymore
because chanels might also be skipped now becauese the are not selected
in the category range.

Also, a new unit test is implemented that verifies the `RooFit::Range()`
command argument for fitTo can be used to select specific components
from a RooSimultaneous.

Closes issue root-project#8231.
guitargeek added a commit to guitargeek/root that referenced this issue Nov 28, 2022
It is possible to set ranges for a `RooCategory`, suggesting that this
is a possible way to select only a subset of channels in a simultaneous
fit. However, this was not working so far, and this commit implements
that.

This commit implements that feature for both the old test statistic
classes and the new BatchMode.

As now it also makes sense to fit to a range that is not defined for the
observables but for the categories only, the message that is printed
when your observables don't define the range is demoted from a warning
to an info message.

The debug message that was printed when channels are not selected also
got removed, because it had some overhead from `sumEntries`, the
debugging prints are rarely used, and the message is not true anymore
because chanels might also be skipped now becauese the are not selected
in the category range.

Also, a new unit test is implemented that verifies the `RooFit::Range()`
command argument for fitTo can be used to select specific components
from a RooSimultaneous.

Closes issue root-project#8231.
guitargeek added a commit that referenced this issue Nov 29, 2022
It is possible to set ranges for a `RooCategory`, suggesting that this
is a possible way to select only a subset of channels in a simultaneous
fit. However, this was not working so far, and this commit implements
that.

This commit implements that feature for both the old test statistic
classes and the new BatchMode.

As now it also makes sense to fit to a range that is not defined for the
observables but for the categories only, the message that is printed
when your observables don't define the range is demoted from a warning
to an info message.

The debug message that was printed when channels are not selected also
got removed, because it had some overhead from `sumEntries`, the
debugging prints are rarely used, and the message is not true anymore
because chanels might also be skipped now becauese the are not selected
in the category range.

Also, a new unit test is implemented that verifies the `RooFit::Range()`
command argument for fitTo can be used to select specific components
from a RooSimultaneous.

Closes issue #8231.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants