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.

Sign up for GitHub

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

Jump to bottom

[RF] ExternalConstraints documentation incorrect for RooMCStudy #15405

Closed
goi42 opened this issue May 2, 2024 · 1 comment · Fixed by #15682
Closed

[RF] ExternalConstraints documentation incorrect for RooMCStudy #15405

goi42 opened this issue May 2, 2024 · 1 comment · Fixed by #15682
Assignees
@guitargeek
Labels
improvement in:RooFit/RooStats
Projects
Fixed in 6.34.00 Fixed in 6.32.02

Comments

@goi42
Copy link
Contributor

goi42 commented May 2, 2024

Explain what you would like to see improved and how.

The documentation for the RooMCStudy constructor says for the argument ExternalConstraints: "Apply internal constraints on given parameters in fit and sample constrained parameter values from constraint p.d.f for each toy." This is not correct. Looking at the source code, it appears that the ExternalConstraints are simply added to the fit options that are eventually passed internally to fitTo.

It would be better, I think, to simply eliminate this as an argument to RooMCStudy. It should instead just be allowed as part of the arbitrary FitOptions. This would be more transparent.

ROOT version

v6.32

Installation method

N/A

Operating system

N/A

Additional context

No response
@guitargeek guitargeek changed the title ExternalConstraints documentation incorrect for RooMCStudy [RF] ExternalConstraints documentation incorrect for RooMCStudy May 11, 2024
guitargeek added a commit to guitargeek/root that referenced this issue May 30, 2024
@guitargeek
[RF] Remove documentation for ExternalConstraints option from RooMC…
8d54466 
…Study

As explained in root-project#15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes root-project#15405.
guitargeek added a commit to guitargeek/root that referenced this issue May 30, 2024
@guitargeek
[RF] Remove docs for ExternalConstraints option from RooMCStudy
01837de 
As explained in root-project#15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes root-project#15405.
@guitargeek guitargeek mentioned this issue May 30, 2024
Merged
@guitargeek
Copy link
Contributor

Thanks for noticing this! I have opened a PR to update the docs. However, I kept the logic of forwarding to the fit options for backwards compatibility.

guitargeek added a commit to guitargeek/root that referenced this issue May 30, 2024
@guitargeek
[RF] Remove docs for ExternalConstraints option from RooMCStudy
7cb390c 
As explained in root-project#15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes root-project#15405.
guitargeek added a commit that referenced this issue May 30, 2024
@guitargeek
[RF] Remove docs for ExternalConstraints option from RooMCStudy
cd1dce9 
As explained in #15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes #15405.
@guitargeek guitargeek modified the milestone: 6.32/02 May 30, 2024
@guitargeek guitargeek added this to Issues in Fixed in 6.34.00 via automation May 30, 2024
@guitargeek guitargeek added this to Issues in Fixed in 6.32.02 via automation May 30, 2024
guitargeek added a commit to guitargeek/root that referenced this issue Jun 1, 2024
@guitargeek
[RF] Remove docs for ExternalConstraints option from RooMCStudy
3a54206 
As explained in root-project#15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes root-project#15405.
guitargeek added a commit that referenced this issue Jun 1, 2024
@guitargeek
[RF] Remove docs for ExternalConstraints option from RooMCStudy
4589fbf 
As explained in #15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes #15405.
PPaye pushed a commit to PPaye/root that referenced this issue Jun 3, 2024
@guitargeek
[RF] Remove docs for ExternalConstraints option from RooMCStudy
20e14ac 
As explained in root-project#15405, the documentation for the `ExternalConstraints`
option in RooMCStudy is misleading. People should just pass this to the
`FitOptions`, so it actually needs no own docs.

The logic to forward the `ExternalConstraints` to the FitOptions is kept
for backwards compatibility.

Closes root-project#15405.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@guitargeek guitargeek

Labels
improvement in:RooFit/RooStats
Projects
Fixed in 6.32.02
Issues
Fixed in 6.34.00
Issues
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[RF] Remove docs for ExternalConstraints option from RooStudy guitargeek/root
2 participants
@guitargeek @goi42