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 Neyman threshold when changing runner_args #100

Merged
merged 13 commits into from
Nov 2, 2023
Merged

Conversation

hammannr
Copy link
Collaborator

For shape parameters such as the WIMP mass, we haven't implemented anything that would allow resetting its nominal value after initialization. However, this is done to obtain the poi_rate_multiplier for a given poi_expectation, which means that the poi_rate_multiplier values corresponding to a given threshold might be wrong in the end.
I did the following to fix this issue and prevent us from running into a similar issue in the future:

  • Initialize a runner for each runner_arg in the NeymanConstructor. This takes a bit longer and may sometimes be overkill but it works.
  • Introduce a derived boolean property for a parameter called static (see Dealing with static shape parameters #81 ), which is true if the parameter is
    • not fittable
    • neither a livetime nor rate (ptype)
    • has no blueice anchors
  • If one tries to override the nominal value of a static parameter or call parameters with a value for it that is different to its nominal value, alea will throw an error.

@hammannr
Copy link
Collaborator Author

I think it would be really neat if the static flag would set itself though I found that this is not really practical for our simple examples and thus I went back to making it a hand-settable flag.

@github-actions
Copy link

github-actions bot commented Oct 30, 2023

Pull Request Test Coverage Report for Build 6689997230

  • 14 of 16 (87.5%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 91.468%

Changes Missing Coverage Covered Lines Changed/Added Lines %
alea/parameters.py 8 10 80.0%
Files with Coverage Reduction New Missed Lines %
alea/model.py 1 85.26%
alea/parameters.py 2 88.1%
Totals Coverage Status
Change from base Build 6652053859: -0.3%
Covered Lines: 1340
Relevant Lines: 1465

💛 - Coveralls

@hammannr hammannr added the bug Something isn't working label Oct 30, 2023
@hammannr hammannr marked this pull request as ready for review October 30, 2023 08:16
@hammannr
Copy link
Collaborator Author

In principle one could also get around loading the runner explicitly by just obtaining the poi_expectation and poi_value from results[true_name], right? I didn't do that now because I wanted to change as little as possible but is there a reason why we should not do that @dachengx ?


self._check_ll_and_generate_data_signature()
self.set_nominal_values(**kwargs.get("nominal_values", {}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the nearby functionality different from simply using set_nominal_values? @hammannr

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 had to change it since otherwise the static parameters would complain if the nominal value were updated in this step

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the outcome is the same. It only had to be changed to accommodate the static parameter functionality

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then maybe set_nominal_values is not checked by pytest. Would you add a test to increase the coverage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, now it's no longer in use so I would suggest to simply trash the function

@dachengx
Copy link
Collaborator

dachengx commented Nov 2, 2023

In principle one could also get around loading the runner explicitly by just obtaining the poi_expectation and poi_value from results[true_name], right? I didn't do that now because I wanted to change as little as possible but is there a reason why we should not do that @dachengx ?

Logically the thresholds json should be controlled by runner configuration yaml but not the files stored. But it is still beneficial to check the consistency between result and calculated rate multiplier?

Better put this in another PR, as an enhancement.

@hammannr
Copy link
Collaborator Author

hammannr commented Nov 2, 2023

For some reason, pre-commit complains about _uncertainty in parameters, which I didn't touch:

alea/parameters.py:78: error: Cannot determine type of "_uncertainty"  [has-type]
alea/parameters.py:79: error: Cannot determine type of "_uncertainty"  [has-type]
alea/parameters.py:81: error: Cannot determine type of "_uncertainty"  [has-type]

@dachengx
Copy link
Collaborator

dachengx commented Nov 2, 2023

For some reason, pre-commit complains about _uncertainty in parameters, which I didn't touch:

alea/parameters.py:78: error: Cannot determine type of "_uncertainty"  [has-type]
alea/parameters.py:79: error: Cannot determine type of "_uncertainty"  [has-type]
alea/parameters.py:81: error: Cannot determine type of "_uncertainty"  [has-type]

I will try to fix it.

@dachengx dachengx changed the title Threshold bug Neyman threshold not updated when changing runner_args Nov 2, 2023
@dachengx dachengx changed the title Neyman threshold not updated when changing runner_args Update Neyman threshold when changing runner_args Nov 2, 2023
@hammannr hammannr merged commit 92c8f14 into main Nov 2, 2023
7 checks passed
@hammannr hammannr deleted the threshold_bug branch November 2, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants