Skip to content

shape paramEffect handling for TemplateSample #28

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

Open
stalbrec wants to merge 1 commit into
base: master
Choose a base branch
from
Open

shape paramEffect handling for TemplateSample #28

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions rhalphalib/sample.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ def setParamEffect(self, param, effect_up, effect_down=None, scale=None):
print("effect_up ({}, {}) has magnitude greater than 50% ({:.2f}%), "
"you might be passing absolute values instead of relative"
.format(param.name, self._name, _weighted_effect_magnitude * 100))
self._paramEffectsUp[param] = effect_up

if effect_down is not None:
if isinstance(effect_down, np.ndarray):
Expand Down Expand Up @@ -226,6 +225,7 @@ def setParamEffect(self, param, effect_up, effect_down=None, scale=None):
self._paramEffectsDown[param] = effect_down
else:
self._paramEffectsDown[param] = None
self._paramEffectsUp[param] = effect_up

if isinstance(scale, numbers.Number):
if isinstance(effect_up, DependentParameter):
Expand All @@ -246,7 +246,12 @@ def getParamEffect(self, param, up=True):
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

@nsmith- nsmith- Jun 8, 2023

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

@nsmith- nsmith- Jul 18, 2023

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

@stalbrec stalbrec Jul 19, 2023

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.

zerobins = (self._nominal <= 0.)
effect_down = self._nominal
effect_down[zerobins] = 1.0
effect_down[~zerobins] -= abs(self._nominal - self._paramEffectsUp[param])[~zerobins]
effect_down[~zerobins] /= self._nominal[~zerobins]
return effect_down
else:
raise NotImplementedError
return self._paramEffectsDown[param]
Expand Down