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

Add 2D plotting script PostFit2DShapesFromWorkspace.cpp #327

Merged
merged 7 commits into from
Jul 22, 2024
Merged

Add 2D plotting script PostFit2DShapesFromWorkspace.cpp #327

merged 7 commits into from
Jul 22, 2024

Conversation

mroguljic
Copy link
Contributor

Resurrection of this PR. I just checked that it compiles on el9.
@ajgilbert your comment on code duplication still stands, but it has been some time since the linked PR and I'm not sure if these functionalities have been added.

ajgilbert
ajgilbert requested changes Jul 1, 2024
View reviewed changes
Copy link
Collaborator

@ajgilbert ajgilbert left a comment

Choose a reason for hiding this comment

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

Hi,

The code introduces a lot of duplication, which is not ideal. That said, it can be merged pending a number of issues (see the code comments for details).

CombineTools/src/CombineHarvester.cc
std::string var_name_y;
if (data_obj) {
var_name = data_obj->get()->first()->GetName();
RooArgSet* temp_vars = (RooArgSet*)data_obj->get()->Clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we own the memory for temp_vars here? Need a delete somewhere?

CombineTools/src/CombineHarvester.cc Outdated
RooArgSet* temp_vars = (RooArgSet*)data_obj->get()->Clone();
RooAbsArg* temp_xvar = data_obj->get()->first();
temp_vars->remove(*temp_xvar,true,true);
var_name_y = temp_vars->first()->GetName();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we not need to check the data object actually has two observables first? Won't this crash if we only have one?

CombineTools/src/CombineHarvester.cc Outdated
entry->set_observable(
(RooRealVar*)entry->pdf()->findServer(var_name.c_str()));
}
entry->set_observable_y(
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, surely we only want to call this if there is a 2nd observables?

CombineTools/interface/Process.h
@@ -79,6 +84,7 @@ class Process : public Object {
RooAbsData* data_;
RooAbsReal* norm_;
RooRealVar* cached_obs_;
RooRealVar* cached_obsy_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Process.cc, need to handle this new data member correctly in all constructors, assignment and swap methods.

CombineTools/src/CombineHarvester_Evaluate.cc
std::string var_name_y = "CMS_th1y";
if (data_obj) {
var_name = data_obj->get()->first()->GetName();
RooArgSet* temp_vars = (RooArgSet*)data_obj->get()->Clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check memory ownership of cloned object

@mroguljic
Copy link
Contributor Author

Thank you for the review @ajgilbert
The PR was updated per your comments. I tested it on my usual 2D workflows, do you have some workflows that I could run to ensure it does not break the typical 1D case?

@ajgilbert
Copy link
Collaborator

Thanks, I think you could run on any set of 1D cards. For example there are some in the tutorials section: https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/tree/main/data/tutorials/htt/125

@mroguljic
Copy link
Contributor Author

I ran the following:

combine -M FitDiagnostics datacard_part2.txt -m 800 --saveShapes --saveWithUncertainties
text2workspace.py datacard_part2.txt -m 800 -o 
workspace_part2.root
PostFitShapesFromWorkspace -d datacard_part2.txt  --output test_shapes.root -m 800 -f fitDiagnosticsTest.root:fit_s --postfit --sampling --print -w workspace_part2.root

where the datacard_part2.txt is slightly changed from the one in Combine repository tutorial. The output plots look reasonable. I'm attaching the datacard and a screenshot of one of the output plots (cannot upload .root files here)
image

mroguljic
mroguljic commented Jul 10, 2024
View reviewed changes
Copy link
Contributor Author

@mroguljic mroguljic left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Implemented the requested changes:

  • add a flag to ensure that we run 2D part of the code only if we have more than one observables
  • delete temporarily needed objects
  • update Process.cc to properly handle new class members

@mroguljic mroguljic requested a review from ajgilbert July 10, 2024 09:53
@ajgilbert ajgilbert merged commit 75e7c7d into cms-analysis:main Jul 22, 2024
@lcorcodilos
Copy link

🎉 Thanks for getting this over the line @mroguljic and @ajgilbert! And glad to see it's still providing value 😄

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

@ajgilbert ajgilbert ajgilbert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mroguljic @ajgilbert @lcorcodilos