-
Notifications
You must be signed in to change notification settings - Fork 393
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
Update Impacts.py #996
Update Impacts.py #996
Conversation
anigamova
commented
Jul 23, 2024
- Adding possibility to process regex in --setParameters
- Remove old iterators (createIterator()) in Impacts.py and FastScan.py
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 Aliya. As well as the minor comment above, I noticed its not entirely clear to me what the intended difference between--setParameters
and --setPhysicsModelParameters
is. It looks like its maybe a relic of times long past?
There are references to it in comments of src/Combine.cc
but these also seem to be mistaken and should be referring to --setParameters
.
python/tool_base/Impacts.py
Outdated
set_parameters_str += var_str + "," | ||
else: | ||
set_parameters_str += setParam + "," | ||
self.args.setPhysicsModelParameters = set_parameters_str.rstrip(",") |
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 think this is meant to be inside the if block starting on l.208, or else set_parameter_str
doesn't exist.
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 spotting this!
I think |
It seems to fail if the option is used since it doesn't actually exist in combine but the tool tries to pass it through. But I can remove it in another PR. This one looks good to me. |
ok, I removed the |
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.
Perfect, thanks!