Skip to content

[RF] additional public methods for accessing/modifying product factors #12048

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
Merged
guitargeek merged 2 commits into from
Jan 19, 2023
Merged

[RF] additional public methods for accessing/modifying product factors #12048

guitargeek merged 2 commits into from
Jan 19, 2023

Conversation

@will-cern
Copy link
Contributor

@will-cern will-cern commented Jan 17, 2023

Adds public methods to allow access and modification of the underlying factors of RooProdPdf and RooProduct

@phsft-bot
Copy link

phsft-bot commented Jan 17, 2023

Can one of the admins verify this patch?

@jalopezg-git jalopezg-git changed the title additional public methods for accessing/modifying product factors [RF] additional public methods for accessing/modifying product factors Jan 17, 2023
@guitargeek
Copy link
Contributor

guitargeek commented Jan 17, 2023

@phsft-bot build

@phsft-bot
Copy link

phsft-bot commented Jan 17, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

guitargeek
guitargeek requested changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Will, thanks for these nice suggestions!

I have one request: the code you implemented is a duplicate of what is already in the RooProdPdf constructor. Can you maybe re-implement the constructor that takes a collection of PDFs in terms of your new addPdfs() function to avoid this? Like that, we also get huge test coverage for free.

guitargeek
guitargeek reviewed Jan 18, 2023
View reviewed changes
guitargeek
guitargeek reviewed Jan 18, 2023
View reviewed changes
@guitargeek
Copy link
Contributor

guitargeek commented Jan 19, 2023

@phsft-bot build

@phsft-bot
Copy link

phsft-bot commented Jan 19, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

guitargeek
guitargeek approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, LGTM!

@guitargeek guitargeek merged commit 8b6d4bb into root-project:master Jan 19, 2023
This was referenced Jan 19, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guitargeek guitargeek guitargeek approved these changes

@lmoneta lmoneta Awaiting requested review from lmoneta

@hageboeck hageboeck Awaiting requested review from hageboeck

Assignees

@guitargeek guitargeek

Labels

in:RooFit/RooStats

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@will-cern @phsft-bot @guitargeek @jalopezg-git