Skip to content

feat: allow getExpectation(eval=True) for samples with independent params; allow shapeU#49

Merged
nsmith- merged 4 commits intonsmith-:masterfrom
andrzejnovak:evals
May 14, 2025
Merged

feat: allow getExpectation(eval=True) for samples with independent params; allow shapeU#49
nsmith- merged 4 commits intonsmith-:masterfrom
andrzejnovak:evals

Conversation

@andrzejnovak
Copy link
Collaborator

@andrzejnovak andrzejnovak commented Apr 1, 2025

This seems to be needed for models with non-trivial nuisances for eval to work. Otherwise the following error gets raised.

File ~/cat/rhalphalib/src/rhalphalib/sample.py:396, in TemplateSample.getExpectation(self, nominal, eval)
    394 else:
    395     effect_down = self.getParamEffect(param, up=False)
--> 396     smoothStep = SmoothStep(param_scaled)
    397     if param.combinePrior == "shape":
    398         combined_effect = smoothStep * (1 + (effect_up - 1) * param_scaled) + (1 - smoothStep) * (1 - (effect_down - 1) * param_scaled)

File ~/cat/rhalphalib/src/rhalphalib/parameter.py:321, in SmoothStep.__init__(self, param)
    319     raise ValueError("Expected a Parameter instance, got %r" % param)
    320 if param.intermediate:
--> 321     raise ValueError("SmoothStep can only depend on a non-intermediate parameter")
    322 self.original_name = param.name
    323 super(SmoothStep, self).__init__(param.name + "_smoothstep", "{0}", param)

ValueError: SmoothStep can only depend on a non-intermediate parameter

HOWEVER, I am not sure that it doesn't break anything else, because a change for existing workflows was required.

This PR fixes getExpectation(eval=True) to work with more complicated sets of nuisances. A cross check comparing values from fitDiagnostics and getExpectation is here:

image

@nsmith-
Copy link
Owner

nsmith- commented Apr 1, 2025

If I recall, the _paramEffectScales is to recreate doing things like myParam shape 2.0 - - in a datacard, where it allows to create a "magnified" shape effect and then scale it back as appropriate. The assumption being the magnified templates might be easier to interpolate between than the original. So this change would appear to break that, on first glance. Or maybe the issue is that smoothStep never worked with such cases but there was no warning/error?

@andrzejnovak
Copy link
Collaborator Author

This PR does not change the default behaviour in combine (evaluating in fitDiagnostics/shapes_prefit)
image

The behaviour when passing extra scaling in setParamEffect(...., 0.5) is unchanged

image

BUT the behaviour currently in for extra scaling doesn't behave correctly in the first place.

image

@andrzejnovak
Copy link
Collaborator Author

https://gist.github.com/andrzejnovak/cdef1484aae873076e83967cd5250a6c Here's a reproducer if you want to take a look @nsmith- / @rkansal47

@andrzejnovak
Copy link
Collaborator Author

@nsmith- fixed up, sorry for some reason this fell of my radar

@nsmith- nsmith- merged commit f901ebc into nsmith-:master May 14, 2025
9 checks passed
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