Skip to content
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 hypotheses and common_hypothesis by pre_process_poi #135

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

dachengx
Copy link
Collaborator

Previously only the generate_values are updated by update_poi:

alea/alea/runner.py

Lines 172 to 181 in cee1e71

def generate_values(self, value: Dict[str, float]) -> None:
if not all([isinstance(v, (float, int)) for v in value.values()]):
raise ValueError(f"generate_values should be a dict of float! But {value} is provided.")
# update poi according to poi_expectation
if "poi_expectation" in value:
self.input_poi_expectation = True
value = self.update_poi(self.model, self.poi, value, self.nominal_values)
else:
self.input_poi_expectation = False
self._generate_values = value

This PR also updates "poi_expectation" in hypotheses and common_hypothesis.

@dachengx dachengx requested review from hammannr and kdund January 22, 2024 18:56
Copy link

github-actions bot commented Jan 22, 2024

Pull Request Test Coverage Report for Build 7640880716

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 90.898%

Totals Coverage Status
Change from base Build 7583977931: 0.3%
Covered Lines: 1438
Relevant Lines: 1582

💛 - Coveralls

@dachengx dachengx marked this pull request as ready for review January 23, 2024 02:06
hammannr
hammannr previously approved these changes Jan 24, 2024
Copy link
Collaborator

@hammannr hammannr left a comment

Choose a reason for hiding this comment

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

Thanks a lot @dachengx for this quick implementation! I tried it with a few toys and it works flawlessly. I left two minor comments that you can implement if you find them useful, otherwise just go ahead and merge 😊

alea/runner.py Show resolved Hide resolved
alea/runner.py Outdated
@@ -164,33 +164,48 @@ def __init__(
statistical_model_args.get("asymptotic_dof", 1),
)

def pre_process_poi(self, value, name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

name is a bit generic. Maybe something like called_by_name instead to make it a bit clearer? But I can also live with name :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated it to attribute_name.

@dachengx dachengx merged commit a2ffc39 into main Jan 24, 2024
7 checks passed
@dachengx dachengx deleted the update_hypo branch January 24, 2024 13:49
@dachengx dachengx added the enhancement New feature or request label Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants