-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: Add setup for custom modifiers #1625
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @lukasheinrich — it is obviously quite important.
This is big enough that I think I'm going to need some help understanding it all, so take most of this review as questions rather than request. But I think a lot of that can be simplified if I understand if my suggestion of a modifier_builder
class makes sense here or not.
Also, I think that all of the notebook changes can get removed here, right? I don't think that anything in this PR is actually showing up as new information/content there and that all the changes are just from you re-running the notebooks. Can you confirm that? ReviewNB isn't showing changes in its visualized diff.
80fc294
to
4cf8f7b
Compare
1b19af7
to
0bdb8e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say I'm pretty impressed. I'd consider this an impactful change that doesn't require a major (or minor) breaking version of pyhf at all.
I can already see how we plan to pull in new modifiers on top of this. I have a few comments overall that I'll mention here:
required_parset
has bothered me on some level, because we do need it to have the same signature across all modifiers, even if the arguments are not being used. I see why it's done this way (so we don't need to handle if/else statements...) but I truly wonder if there's a better way to co-locate/define these "defaults" in an easy way for users to look up.- the builder/combined classes are great, do we want to do something similar to how our optimizer works, where users can inherit from a base class and provide the necessary functions in order to inject their custom mod in? This would also align similar to how coffea/uproot does things as well. I'm happy to add on top of this if we merge into master as is. (On some level, the way we had a modifier registry allowed users to inject their own modifiers using the decorators, but I guess we're moving away from decorators?)
- in some places, you use mega_mods, in some places, mega_samples, builder_data, and in some places, "modifiers". it's not clear to me which is what or if they are all meant to be the same.
- why do we drop
workspace.parameters
andmodel.parameters
entirely? Is this seen as duplicated by the similarmodifiers
attribute?
a907f6b
to
d004e27
Compare
for more information, see https://pre-commit.ci
This looks very promising, thanks for all the work! I noticed one thing when testing the downstream effects: the order of model parameters can change. While this may not be a breaking change for many users (the order is something that should probably not be relied on anyway), it may cause some unexpected effects. I think it would be useful to highlight this change (assuming it is an intended side-effect of this PR). |
Hi @alexander-held: yes - while @kratsg is right that in principle this could be sold as a pure refactoring i'd vote for a minor version since this is a pretty big change |
Passes on all CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all of this @lukasheinrich. 🚀
If people are happy with the docstring changes I made, then this all LGTM.
* Fix a regression introduced in PR #1625 * Check that the poi set is an existing parameter in the model * Add tests for custom modifiers to cover the use case and make sure it's caught
…sample data (#1708) * Add a check to bin-wise modifiers to ensure that the modifier data length is equal to the length of the data in the sample containing the modifier. If this fails, raise a pyhf.exceptions.InvalidModifier and try to give the user as much information as possible about what modifier has failed. - Apply this to histosys, shapesys, and staterror * Add tests to test_modifiers for this modifier behavior. * Apply isort to all changed files * Remove unused testing code from test_modifiers.py that was missed for removal in PR #1625
* Add checks to determine if modifiers should be shared across channels when the modifier is being built. This guards against accidental correlations across channels. e.g. shared shapesys paramsets. - Amends PR #1625 * Add tests for raised pyhf.exceptions.InvalidModel in the event that a shared shapesys paramset is introduced in the model spec.
Description
Closes #1188
a cleaner version of PR #1188 on the way to addressing Issue #850
Before Merging
For the PR Assignees: