Skip to content

Conversation

@stalbrec
Copy link
Collaborator

@stalbrec stalbrec commented Mar 2, 2023

Hi,

I encountered a apparently rare situation when building TemplateSamples with shape nuisances using histograms as up/down effect, where both the norm of the nominal and up/down hists is ~1.

First part is that paramEffectUp is saved even if the effect is nil (i.e. np.all(effect_up==1)) whenever one gives also something not None as effect_down even if np.all(effect_down==1). Thats why I moved the line self._paramEffectsUp[param] = effect_up down to the point where the check if the down-variation is different from 1 has been done.

Second part is in the symmetrisation of the paramEffect if effect_down is None for a parameter. In the function setParamEffect effects for a shape param are stored as relatives but here it is handled as absolutes, if i understand this correctly.
So whenever one would provide only the param up-effect the constructed down-effect could be some bogus negative array. Which happened to me before i fixed the first part.

Does it make sense to fix it like this, or am i missing something?

if param.combinePrior == 'lnN':
return 1. / self._paramEffectsUp[param]
elif param.combinePrior == 'shape':
return self._nominal - abs(self._nominal - self._paramEffectsUp[param])
Copy link
Owner

Choose a reason for hiding this comment

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

For shape the effect is truly additive rather than multiplicative as far as I remember. So I don't understand the last divide by nominal step

Copy link
Owner

Choose a reason for hiding this comment

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

Did you get a chance to check this? Or do I misunderstand?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for the late response.
I did not have a chance to look into this again. I will try to do so this week.

@nsmith-
Copy link
Owner

nsmith- commented Jun 8, 2023

Sorry for the delay. I for some reason don't get notifications on this library

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

Successfully merging this pull request may close these issues.

2 participants