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

Problem with response histogram getting filled twice? #53

Closed
hschellman opened this issue Sep 17, 2022 · 10 comments
Closed

Problem with response histogram getting filled twice? #53

hschellman opened this issue Sep 17, 2022 · 10 comments
Assignees

Comments

@hschellman
Copy link
Collaborator

There seems to be something going on that fills both the tuned and untuned responses with the same thing. This leads to there being too many entries in both. May be how pointers to the responses are set up. Basically tuned and untuned may be sharing the same pointer so you fill both.

@hschellman
Copy link
Collaborator Author

I think it is partially because the Fill methods don't know if they are getting a tuned or untuned weight. Fill methods should probably get a isTuned input variable and the loops code should call with and without that set.
This goes for all fill methods.
So if you are running in both mode, it looks as if Loops puts weights into both responses?

@hschellman
Copy link
Collaborator Author

It appears that the code wasn't filling tuned and untuned responses separately - they had the same content. I'm thinking of adding a scale argument to the various fillers so that one can choose whether or not to use it. First test indicates that, at least, the responses are different when you do this. But the treatment of responses differently from HistWrappers does make this hard to track down. We need a class diagram of all of this stuff.

@hschellman
Copy link
Collaborator Author

Yep, both Fillers.h and the VariablesFromConfig classes were deciding to fill the Response hist, I made the Fillers.h just call the VariablesFromConfig methods which then dealt with the tuned/untuned part. This is now in branch hms-2022-09 and 2D histograms now agree with 1D for ptmu.

@hschellman
Copy link
Collaborator Author

Ways to check for this. Integral of the mcsighist should be close to the integral of the migration matrix. It was off by a factor of 2. Also the tuned and untuned versions need to have different integrals.

@physnerds
Copy link

physnerds commented Sep 18, 2022 via email

@hschellman
Copy link
Collaborator Author

This is specific to the implementation for our MAT. I asked Noah to do it "both ways" and unfortunately, it bother twice. But an interesting feature is that RooUnfold really gets grumpy if the response matrix is not the same normalization as the mc histograms and basically throws in a factor of 2 for the fun of it.

@hschellman
Copy link
Collaborator Author

Did both twice, not bother.

@nhvaughan nhvaughan self-assigned this Sep 20, 2022
@hschellman
Copy link
Collaborator Author

Ok Noah, I think the solution was to do things right and move the response creation into the VariableFromConfig methods. That actually makes it possible not to have the new constructor (I just put in) for the histwrappermap that had to have both reco and truth binning in it. So will be a major cleanup in the code. Apologies for ever designing it this way.

@nhvaughan
Copy link
Contributor

response-fix has 1D going. 2D in the works, which should go much faster.

@nhvaughan
Copy link
Contributor

2D is done. Would like another test from someone else before merging to main

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

No branches or pull requests

3 participants