First runner manipulating statistical model #50

dachengx · 2023-07-21T17:47:02Z

Trying to resolve #3 and #16

The docstring of Runner will be rendered after #68.

What does the code in this PR do / what does it improve?

The runner manipulates the statistical model.

It:

initializes the statistical model
generates or reads toy data
saves toy data if needed
fits parameters
writes the output file

Can you briefly describe how it works?

The changes except in runner.py, models, and model.py is only some format changes, not crucial.

likelihood_config should be contained in statistical_model_args if needed.
fit_guesses is defined in Parameters.

Define a store_data method of BlueiceExtendedModel, to save the generate_values. Because store_data of StatisticalModel only saves likelihood_names in the data_name_list by default.
The updating of the ancillary terms is implemented when you set self.statistical_model.data.
Add keyword arguments best_fit_args and confidence_interval_args to method StatisticalModel.confidence_interval. best_fit_args gives you the global best fit. But when calculating CL, the hypothesis can be different from the best fit, so you need confidence_interval_args.

Can you give a minimal working example (or illustrate with a figure)?

from alea.runner import Runner

parameter_definition = {
    'mu': {
        'fit_guess': 0.0,
        'fittable': True,
        'nominal_value': 0.0,
        'parameter_interval_bounds': [
            -10,
            10
        ],
    },
    'sigma': {
        'fittable': False,
        'nominal_value': 1.0,
    }
}

toydata-generating runner:

generate_runner = Runner(
    statistical_model='alea.examples.gaussian_model.GaussianModel',
    parameter_definition=config['parameter_definition'],
    compute_confidence_interval=True,
    poi='mu',
    hypotheses=['free', 'null', 'true'],
    true_generate_values={'mu': 1., 'sigma': 1.},
    n_mc=3,
    toydata_file=toydata_file,
    toydata_mode='generate_and_write',
    output_file='test_toymc_generate.hdf5',
)
generate_runner.run()

toydata-reading runner:

read_runner = Runner(
    statistical_model='alea.examples.gaussian_model.GaussianModel',
    parameter_definition=config['parameter_definition'],
    compute_confidence_interval=True,
    poi='mu',
    hypotheses=['free', 'null', 'true'],
    true_generate_values={'mu': 1., 'sigma': 1.},
    n_mc=3,
    toydata_file=toydata_file,
    toydata_mode='read',
    output_file='test_toymc_read.hdf5',
)
read_runner.run()

What are the potential drawbacks of the codes?

Things should be implemented later:

confidential intervals
update fit_guess of Parameters

Please include the following if applicable:

Update the docstring(s)
Update the documentation
Tests to check the (new) code is working as desired.
Does it solve one of the open issues on github?

Notes on testing

Until the automated tests pass, please mark the PR as a draft.

All italic comments can be removed from this template.

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

pep8

alea/runner.py|211 col 1| F811 redefinition of unused 'StatisticalModel' from line 11
alea/runner.py|211 col 1| E402 module level import not at top of file
alea/runner.py|214 col 1| F811 redefinition of unused 'Runner' from line 14
alea/runner.py|214 col 1| WPS338 Found incorrect order of methods in a class
alea/runner.py|214 col 1| WPS230 Found too many public instance attributes: 14 > 6
alea/runner.py|271 col 9| WPS414 Found incorrect unpacking target
alea/runner.py|271 col 9| WPS414 Found incorrect unpacking target
alea/runner.py|271 col 9| WPS414 Found incorrect unpacking target
alea/runner.py|281 col 1| E800 Found commented out code
alea/runner.py|282 col 1| E800 Found commented out code
alea/runner.py|283 col 1| E800 Found commented out code
alea/runner.py|318 col 9| WPS221 Found line with high Jones Complexity: 15 > 14
alea/runner.py|320 col 22| WPS510 Found in used with a non-set container
alea/runner.py|328 col 48| WPS441 Found control variable used after block: ea

alea/blueice_extended_model.py

alea/runner.py

kdund · 2023-07-21T17:50:40Z

alea/runner.py

+            confidence_interval_threshold: Callable[[float], float] = None,
+            poi: str = None,
+            hypotheses: list = None,
+            common_generate_values: dict = None,


What is the distinction between common_generate_values and true_generate_values?
the generate values are always true for the datasets generated with them, no?

updated in previous commits.

kdund · 2023-07-21T17:50:59Z

alea/runner.py

+        #     pass
+        return parameter_list, result_list, result_dtype
+
+    def _get_generate_values(self):


The naming inside here mixed hypothesis and generate values even for when hypothesis is not "true"-- I think this is unfortunate, separating them conceptually is a key to getting the concepts right in my mind.

updated in previous commits.

alea/runner.py

github-actions · 2023-07-22T20:24:57Z

Pull Request Test Coverage Report for Build 5744366197

167 of 171 (97.66%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+9.2%) to 72.302%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
alea/parameters.py	18	19	94.74%
alea/utils.py	8	9	88.89%
alea/model.py	24	26	92.31%

Files with Coverage Reduction	New Missed Lines	%
alea/model.py	1	86.47%

Totals
Change from base Build 5730856253:	9.2%
Covered Lines:	757
Relevant Lines:	1047

💛 - Coveralls

alea/models/blueice_extended_model.py

alea/runner.py

Encourage people to use h5 instead of hdf5

tests/test_runner.py

alea/template_source.py

dachengx · 2023-07-31T20:35:58Z

@kdund I am done with your suggestions.

I will then tune the pytest.

alea/runner.py

tests/test_runner.py

alea/model.py

alea/runner.py

alea/parameters.py

alea/runner.py

tests/test_gaussian_model.py

tests/test_runner.py

alea/model.py

alea/parameters.py

alea/runner.py

…on happens

kdund

vvv nice! Some comments

alea/examples/gaussian_model.py

alea/model.py

alea/models/blueice_extended_model.py

kdund · 2023-08-02T20:37:40Z

alea/models/blueice_extended_model.py

@@ -245,12 +264,20 @@ def _generate_data(self, **generate_values) -> dict:
        data["generate_values"] = dict_to_structured_array(generate_values)
        return data

+    def store_data(self, file_name, data_list, data_name_list=None, metadata=None):


I appreciate this here, but I feel like it is a bit troublesome-- we want to say you only need ll, generate_data, but here we're redefining other functionality also, when we could just set the data_name_list at init, I guess?

Do you want to change likelihood_names here to data_name_list:

alea/alea/model.py

Lines 214 to 218 in b710149

if data_name_list is None:

if hasattr(self, "likelihood_names"):

data_name_list = self.likelihood_names

else:

data_name_list = ["{:d}".format(i) for i in range(len(_data_list[0]))]

?

I think the logic is:

ll and generate_data are mandatory for a new model, this is already demonstrated at GaussianModel

BlueiceExtendedModel needs an overwritten store_data function because it has an advantage that generate_values is also in the self.data, so that the StatisticalModel.store_data can not be directly applied on BlueiceExtendedModel.

Someone overwriting store_data, does not violate the truth that only ll and generate_data are mandatory. In principle, one can overwrite everything.

kdund · 2023-08-02T20:41:08Z

alea/parameters.py

+            value = [-MAX_FLOAT, MAX_FLOAT]
+        else:
+            if value[0] is None:
+                value[0] = -MAX_FLOAT


why max float and not inf?

If set the boundary parameter_interval_bounds to np.inf, we will see errors like:

0%| | 0/3 [00:11<?, ?it/s] --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) Cell In[31], line 1 ----> 1 toydata, results = runner.toy_simulation() 2 runner.write_output(results) File ~/alea/alea/runner.py:229, in Runner.toy_simulation(self) 227 fit_result['ll'] = max_llh 228 if self._compute_confidence_interval and (self.poi not in hypothesis_values): --> 229 dl, ul = self.statistical_model.confidence_interval( 230 poi_name=self.poi, 231 best_fit_args=self._hypotheses_values[0], 232 confidence_interval_args=hypothesis_values) 233 else: 234 dl, ul = np.nan, np.nan File ~/alea/alea/model.py:465, in StatisticalModel.confidence_interval(self, poi_name, parameter_interval_bounds, confidence_level, confidence_interval_kind, best_fit_args, confidence_interval_args) 463 if confidence_interval_kind in {"upper", "central"} and t_best_parameter < 0: 464 if t(parameter_interval_bounds[1]) > 0: --> 465 ul = brentq(t, best_parameter, parameter_interval_bounds[1]) 466 else: 467 ul = np.inf File /opt/XENONnT/anaconda/envs/XENONnT_2023.07.2/lib/python3.9/site-packages/scipy/optimize/_zeros_py.py:784, in brentq(f, a, b, args, xtol, rtol, maxiter, full_output, disp) 782 if rtol < _rtol: 783 raise ValueError("rtol too small (%g < %g)" % (rtol, _rtol)) --> 784 r = _zeros._brentq(f, a, b, xtol, rtol, maxiter, args, full_output, disp) 785 return results_c(full_output, r) RuntimeError: Failed to converge after 100 iterations.

This is because scipy.optimize.brentq can not accept np.inf as the boundary. Slack discussion: https://xenonnt.slack.com/archives/C04JRF0AZTP/p1690870295199999

So MAX_FLOAT here is a suggested very large value, > 10^19.

Which will make the brentq super slow-- I think we should rather ask for a sensible range to always be given.

Maybe give a warning?

I guess-- do we set in the test configs?

I would throw an error tbh

Well, even if I change MAX_FLOAT from 10^19 to 100, the iteration time did not change.

Checked by

ul, r = brentq(t, best_parameter, parameter_interval_bounds[1], full_output=True) print(r)

Maybe it is an advantage of brentq.

for the gaussian ex of both? I'm frankly worried about all kinds of numerics also at such high values.

resolved to throw a warning if you do not redefine this

alea/runner.py

tests/test_runner.py

dachengx · 2023-08-02T22:37:44Z

I just added a data generator of Runner, and nothing functionally changed.

kdund

I suggest we postpone per-hypothesis switching on whether to compute confidence intervals for now-- we seldom need it, the neyman computation which takes most of the time has no confidence intervals.

alea/model.py

kdund · 2023-08-02T22:38:22Z

alea/parameters.py

+            value = [-MAX_FLOAT, MAX_FLOAT]
+        else:
+            if value[0] is None:
+                value[0] = -MAX_FLOAT


I guess-- do we set in the test configs?

kdund · 2023-08-02T22:38:34Z

alea/parameters.py

+            value = [-MAX_FLOAT, MAX_FLOAT]
+        else:
+            if value[0] is None:
+                value[0] = -MAX_FLOAT


I would throw an error tbh

kdund · 2023-08-02T22:49:43Z

alea/parameters.py

+            value = [-MAX_FLOAT, MAX_FLOAT]
+        else:
+            if value[0] is None:
+                value[0] = -MAX_FLOAT


for the gaussian ex of both? I'm frankly worried about all kinds of numerics also at such high values.

kdund · 2023-08-03T17:17:25Z

alea/parameters.py

+            value = [-MAX_FLOAT, MAX_FLOAT]
+        else:
+            if value[0] is None:
+                value[0] = -MAX_FLOAT


resolved to throw a warning if you do not redefine this

alea/model.py

kdund

THanks, @dachengx to me this looks good

dachengx added 3 commits July 22, 2023 01:40

Recover runner

232c6de

Improve code style

628d452

Minor change

cc0d06e

github-actions bot reviewed Jul 21, 2023

View reviewed changes

kdund reviewed Jul 21, 2023

View reviewed changes

alea/runner.py Outdated Show resolved Hide resolved

dachengx added 2 commits July 23, 2023 04:17

Merge remote-tracking branch 'origin/main' into first_runner

f869822

Remove duplicated codes

0e334dd

hammannr self-requested a review July 25, 2023 06:47

Merge remote-tracking branch 'origin/main' into first_runner

f4c063e

github-actions bot reviewed Jul 28, 2023

View reviewed changes

alea/models/blueice_extended_model.py Show resolved Hide resolved

alea/models/blueice_extended_model.py Show resolved Hide resolved

Simplify toydata mode

4fb9113

github-actions bot reviewed Jul 31, 2023

View reviewed changes

Add test_runner.py

d31d4bc

Encourage people to use h5 instead of hdf5

github-actions bot reviewed Jul 31, 2023

View reviewed changes

tests/test_runner.py Outdated Show resolved Hide resolved

tests/test_runner.py Outdated Show resolved Hide resolved

tests/test_runner.py Show resolved Hide resolved

alea/template_source.py Show resolved Hide resolved

alea/template_source.py Show resolved Hide resolved

Happier code style

80e0dd6

dachengx marked this pull request as ready for review July 31, 2023 20:35

github-actions bot reviewed Jul 31, 2023

View reviewed changes

Minor change

2c624dc

dachengx requested a review from kdund July 31, 2023 20:38

Help model to save dict data and list data in a unified function

06ad8f3

github-actions bot reviewed Jul 31, 2023

View reviewed changes

dachengx added 2 commits July 31, 2023 18:07

Happier code style

5104568

Minor change at docstring

dec5ca0

github-actions bot reviewed Aug 1, 2023

View reviewed changes

alea/parameters.py Outdated Show resolved Hide resolved

alea/parameters.py Outdated Show resolved Hide resolved

alea/runner.py Show resolved Hide resolved

dachengx added 2 commits July 31, 2023 23:31

Minor change

3823458

Confidential interval calculation

c0e69ff

dachengx force-pushed the first_runner branch from b252c8b to c0e69ff Compare August 1, 2023 08:30

github-actions bot reviewed Aug 1, 2023

View reviewed changes

dachengx added 6 commits August 1, 2023 03:53

Happier code style

8f22c0c

Merge remote-tracking branch 'origin/main' into first_runner

a1b82fe

Merge branch 'main' into first_runner

345f976

Set sigma as not fittable otherwise horrible things when CL calculati…

9e5c79f

…on happens

Remove todo of runner because it is done

89c0bd3

Make the placeholder is already an OK gaussian model example

b710149

kdund requested changes Aug 2, 2023

View reviewed changes

dachengx added 2 commits August 2, 2023 17:20

Few change, add warnings and improve performance

9215c9c

Convert the data generation of runner into a generator

9a577e3

dachengx added 2 commits August 2, 2023 17:55

More compact data generator

7958d71

Also specify what if toydata_mode is no_toydata

6bab21b

kdund approved these changes Aug 3, 2023

View reviewed changes

dachengx added 2 commits August 4, 2023 09:58

Warning when parameter_interval_bounds contains None

3dbaa98

Update docstring a bit

f35e0c7

dachengx linked an issue Aug 4, 2023 that may be closed by this pull request

Add Runner class #16

Closed

kdund approved these changes Aug 4, 2023

View reviewed changes

dachengx merged commit 66538e5 into main Aug 4, 2023

dachengx deleted the first_runner branch August 4, 2023 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First runner manipulating statistical model #50

First runner manipulating statistical model #50

dachengx commented Jul 21, 2023 •

edited

Loading

github-actions bot left a comment

kdund Jul 21, 2023

dachengx Aug 2, 2023

kdund Jul 21, 2023

dachengx Aug 2, 2023

github-actions bot commented Jul 22, 2023 •

edited

Loading

dachengx commented Jul 31, 2023 •

edited

Loading

kdund left a comment

kdund Aug 2, 2023

dachengx Aug 2, 2023

kdund Aug 2, 2023

dachengx Aug 2, 2023 •

edited

Loading

kdund Aug 2, 2023

dachengx Aug 2, 2023

kdund Aug 2, 2023

kdund Aug 2, 2023

dachengx Aug 2, 2023 •

edited

Loading

kdund Aug 2, 2023

kdund Aug 3, 2023

dachengx commented Aug 2, 2023

kdund left a comment

kdund Aug 2, 2023

kdund Aug 2, 2023

kdund Aug 2, 2023

kdund Aug 3, 2023

kdund left a comment

	if data_name_list is None:
	if hasattr(self, "likelihood_names"):
	data_name_list = self.likelihood_names
	else:
	data_name_list = ["{:d}".format(i) for i in range(len(_data_list[0]))]

First runner manipulating statistical model #50

First runner manipulating statistical model #50

Conversation

dachengx commented Jul 21, 2023 • edited Loading

What does the code in this PR do / what does it improve?

Can you briefly describe how it works?

Can you give a minimal working example (or illustrate with a figure)?

What are the potential drawbacks of the codes?

Notes on testing

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5744366197

💛 - Coveralls

dachengx commented Jul 31, 2023 • edited Loading

kdund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dachengx Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dachengx Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dachengx commented Aug 2, 2023

kdund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdund left a comment

Choose a reason for hiding this comment

dachengx commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 22, 2023 •

edited

Loading

dachengx commented Jul 31, 2023 •

edited

Loading

dachengx Aug 2, 2023 •

edited

Loading

dachengx Aug 2, 2023 •

edited

Loading