Simplify TemplateSource, CombinedSource and SpectrumTemplateSource #69

dachengx · 2023-07-31T01:46:33Z

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

The functionality of TemplateSource, CombinedSource, and SpectrumTemplateSource are unchanged.

Add test of CombinedSource.

The weights can be set as shape parameters in the config. You can check it by:

config = load_yaml('unbinned_wimp_statistical_model.yaml')
blueice_model = BlueiceExtendedModel(
    parameter_definition=config['parameter_definition'],
    likelihood_config=config['likelihood_config'],
)

llh_term = blueice_model.get_likelihood_term_from_name('sr2')
zs = np.asarray([-2, 0])

blueice_model._likelihood.likelihood_list[llh_term].mus_interpolator(zs)
blueice_model._likelihood.likelihood_list[llh_term].ps_interpolator(zs).std()

But the SpectrumTemplateSource is not tested, also the configuration is too complex, especially the normalization.

Can you briefly describe how it works?

I already tested that the results are the same in main and simplify_source.

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

Please refer to test_blueice_extended_model.py.

What are the potential drawbacks of the codes?

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?

alea/template_source.py

coveralls · 2023-07-31T01:47:54Z

Pull Request Test Coverage Report for Build 5880564532

121 of 143 (84.62%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+15.5%) to 87.353%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
alea/utils.py	16	17	94.12%
alea/template_source.py	104	125	83.2%

Files with Coverage Reduction	New Missed Lines	%
alea/template_source.py	1	83.68%

Totals
Change from base Build 5878428073:	15.5%
Covered Lines:	891
Relevant Lines:	1020

💛 - Coveralls

alea/template_source.py

alea/utils.py

Because of #70

dachengx · 2023-07-31T06:04:56Z

Maybe we also want to test SpectrumTemplateSource.

alea/template_source.py

dachengx · 2023-07-31T19:30:11Z

This PR depends on #68.

github-actions · 2023-08-01T05:40:06Z

alea/parameters.py

@@ -1,5 +1,7 @@
 from typing import Any, Dict, List, Optional, Tuple

+import scipy


[pep8] _{reported by reviewdog 🐶}
F401 'scipy' imported but unused

Why do you import it though @dachengx ? If there's a reason you can also just put the #noqa: F401 to make clear that it is intentionally added here and nobody accidentally removes it in the future 😊

kdund

SOme comments-- at the moment, the only way I found testing it was to import binference plotting code, is there a better way, you think?

alea/examples/configs/unbinned_wimp_statistical_model.yaml

kdund · 2023-08-01T15:21:14Z

alea/template_source.py

        # ... and finally to probability density
-        if 0 < self.events_per_day:
+        if self.events_per_day > 0:


I can see how it is correct if read in english, but I have in general made many more mistakes when I did not consistently never use > (so all inequalities have the smaller number to the left)

We will apply a < b < c in the future.

kdund · 2023-08-01T15:28:40Z

alea/template_source.py

            ret[n] = t[:, i]
        return ret


-class CombinedSource(blueice.HistogramPdfSource):
+class CombinedSource(TemplateSource, HistogramPdfSource):
    """
    Source that inherits structure from TH2DSource by Jelle,


HistogramPdfSource is the one in blueice originally written by Jelle

I meant rather in the docstring also :)

dachengx · 2023-08-03T05:04:34Z

I think histogram_multiplier is a bad name, actually it is the absolute rate.

hammannr · 2023-08-03T17:22:02Z

I think histogram_multiplier is a bad name, actually it is the absolute rate.

It depends on how you use it and I think we agreed that we try to avoid people using it. But really, the histogram is just multiplied with it, right?

alea/template_source.py

hammannr

I scrolled through the changes and it looks nice, for sure useful to clean up this part. Though a bit hard to keep track and check all the changes. Maybe it would make sense to add a test for the combined source or did I just miss it?
Also, could you maybe think of a case where this source would be used so that we can make a sensible example out of it?

dachengx · 2023-08-04T19:06:15Z

I think histogram_multiplier is a bad name, actually it is the absolute rate.

It depends on how you use it and I think we agreed that we try to avoid people using it. But really, the histogram is just multiplied with it, right?

The histogram is normalized to one and then multiplied by it.

dachengx · 2023-08-04T19:09:13Z

I scrolled through the changes and it looks nice, for sure useful to clean up this part. Though a bit hard to keep track and check all the changes. Maybe it would make sense to add a test for the combined source or did I just miss it? Also, could you maybe think of a case where this source would be used so that we can make a sensible example out of it?

The test it at

alea/alea/examples/configs/unbinned_wimp_statistical_model.yaml

Line 151 in 20b53de

class: alea.template_source.CombinedSource

. But it is not meaningful.

I would postpone the meaningful test to the future when we really used the CombinedSource. Currently, the source is just functionally runnable.

Reference: lemurheavy/coveralls-public#1065 (comment)

dachengx · 2023-08-06T01:40:12Z

histogram_scale_factor is also multiplied on the histogram.

hammannr · 2023-08-09T08:14:15Z

Ah, right @dachengx ! The histogram_multiplier was renamed to histogram_scale_factor in the TemplateSource. For the CombinedSource it indeed is the rate before slicing the way it is implemented. I never worked with the CombinedSource, do we need/want this behaviour? Otherwise we might consider sticking to a histogram_scale_factor for both that do the same thing i.e. just being multiplied to the template. If you want to normalize things to one you should do so yourself. But in any case, the intended usage is to only use the multiplier if you very quickly want to check or vary something. For production you should update the normalization of the template according to the expectation value, at least this is what we discussed with @kdund I think.

hammannr · 2023-08-09T08:16:42Z

Ok I see that for now there is no urgency to have a meaningful test but it would be very useful to have an idea when this source could be used and how one would want to use it, I think. Maybe we could also make this a bit clearer in the docstring since rn I have no idea what to do with it tbh 😄

dachengx · 2023-08-10T19:36:35Z

For this to be merged, we need more example templates like #30 provided. Especially the 3D template with (E, cS2, cS2).

Unless we can live with the nonsense pytest. Currently the config .yaml files only. provide a runnable model definition. But the SpectrumTemplateSource there is nonsense.

dachengx · 2023-08-16T15:12:24Z

@kdund I moved the test of sources into another test module. They will not participate in CL calculation, to save time.

kdund

Thanks, @dachengx I think from my side this looks good now

kdund · 2023-08-07T14:07:18Z

alea/examples/configs/unbinned_wimp_statistical_model.yaml

@@ -56,7 +61,7 @@ parameter_definition:
  er_band_shift:
    nominal_value: 0
    ptype: shape
-    uncertainty: null  # 'scipy.stats.uniform(loc=-1, scale=2)'
+    uncertainty: null  # stats.uniform(loc=0, scale=2).logpdf


I think making all our tests usig the same large config is not optimal-- could we make smaller likelihoods for each aspects of the test?

kdund · 2023-08-07T14:10:34Z

alea/parameters.py

-                raise ValueError(
-                    f"Uncertainty string '{self._uncertainty}'"
-                    " must start with 'scipy.' or 'numpy.'")
+            NotImplementedError(


What is the challenge in not including this functionality for now?

kdund · 2023-08-16T19:43:52Z

README.md


+## Ackgnowledgements


acknowledgements :)

Simplify TemplateSource

ec0bc27

github-actions bot reviewed Jul 31, 2023

View reviewed changes

Update settings of utils, add SR2 to demo CombinedSource

cd0a594

github-actions bot reviewed Jul 31, 2023

View reviewed changes

dachengx added 2 commits July 31, 2023 00:21

Minor change

1f9251b

Set numpy version to less than 1.23.0

789e3f2

Because of #70

dachengx marked this pull request as ready for review July 31, 2023 06:04

dachengx requested a review from kdund July 31, 2023 06:04

dachengx requested a review from hammannr July 31, 2023 06:05

dachengx mentioned this pull request Jul 31, 2023

Unify and clean code style and docstring #68

Merged

Update docstring to Google style

e1aac2a

github-actions bot reviewed Jul 31, 2023

View reviewed changes

Check needed weighted parameters of CombinedSource

ef53ff8

github-actions bot reviewed Aug 1, 2023

View reviewed changes

Update docstring

459c93b

kdund requested changes Aug 1, 2023

View reviewed changes

dachengx added 4 commits August 1, 2023 13:43

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

e1cf100

No need to use multiple inheritance

698e0ba

Merge branch 'main' into simplify_source

c7252e8

Raise error when there is no key in the json file

462fb5b

hammannr reviewed Aug 3, 2023

View reviewed changes

alea/template_source.py Outdated Show resolved Hide resolved

hammannr reviewed Aug 3, 2023

View reviewed changes

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

20b53de

Recover logging, update docstring

de2044d

dachengx requested review from kdund and hammannr August 4, 2023 19:18

Kill cache of coverage

07fb1c0

Reference: lemurheavy/coveralls-public#1065 (comment)

dachengx force-pushed the simplify_source branch from 3b95e5c to 07fb1c0 Compare August 4, 2023 19:21

dachengx added 2 commits August 5, 2023 21:51

Test SpectrumTemplateSource

6fd9c19

Demo future callable uncertainty

1356abb

dachengx mentioned this pull request Aug 8, 2023

slice not working for numpy==1.25.1 #70

Closed

Use latest numpy

52c2681

Rename histogram_multiplier to histogram_scale_factor

bf29d49

dachengx added the testing Works on testing code label Aug 15, 2023

dachengx added 4 commits August 16, 2023 22:38

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

c2b0cf3

Test template source separately in another test module

b3955f3

Remove double quotes if not necessary

b063d6e

Forget to upload this file

4f5508a

kdund added 2 commits August 16, 2023 15:45

tiny typo

68d2a2a

tiny docstring change

72b4e17

kdund approved these changes Aug 16, 2023

View reviewed changes

Fix little typo

acbd7db

dachengx merged commit b61fffd into main Aug 16, 2023
4 checks passed

dachengx deleted the simplify_source branch August 16, 2023 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify TemplateSource, CombinedSource and SpectrumTemplateSource #69

Simplify TemplateSource, CombinedSource and SpectrumTemplateSource #69

dachengx commented Jul 31, 2023 •

edited

Loading

coveralls commented Jul 31, 2023 •

edited

Loading

dachengx commented Jul 31, 2023

dachengx commented Jul 31, 2023

github-actions bot Aug 1, 2023

hammannr Aug 3, 2023

dachengx Aug 4, 2023

kdund left a comment

kdund Aug 1, 2023

dachengx Aug 2, 2023

kdund Aug 1, 2023

dachengx Aug 2, 2023

kdund Aug 3, 2023 •

edited

Loading

dachengx Aug 4, 2023

dachengx commented Aug 3, 2023

hammannr commented Aug 3, 2023

hammannr left a comment

dachengx commented Aug 4, 2023 •

edited

Loading

dachengx commented Aug 4, 2023

dachengx commented Aug 6, 2023

hammannr commented Aug 9, 2023

hammannr commented Aug 9, 2023

dachengx commented Aug 10, 2023 •

edited

Loading

dachengx commented Aug 16, 2023

kdund left a comment

kdund Aug 7, 2023

kdund Aug 7, 2023

kdund Aug 16, 2023

		@@ -1,5 +1,7 @@
		from typing import Any, Dict, List, Optional, Tuple

		import scipy

Simplify TemplateSource, CombinedSource and SpectrumTemplateSource #69

Simplify TemplateSource, CombinedSource and SpectrumTemplateSource #69

Conversation

dachengx commented Jul 31, 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?

coveralls commented Jul 31, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5880564532

💛 - Coveralls

dachengx commented Jul 31, 2023

dachengx commented Jul 31, 2023

github-actions bot Aug 1, 2023

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

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 Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dachengx commented Aug 3, 2023

hammannr commented Aug 3, 2023

hammannr left a comment

Choose a reason for hiding this comment

dachengx commented Aug 4, 2023 • edited Loading

dachengx commented Aug 4, 2023

dachengx commented Aug 6, 2023

hammannr commented Aug 9, 2023

hammannr commented Aug 9, 2023

dachengx commented Aug 10, 2023 • edited Loading

dachengx commented Aug 16, 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

dachengx commented Jul 31, 2023 •

edited

Loading

coveralls commented Jul 31, 2023 •

edited

Loading

kdund Aug 3, 2023 •

edited

Loading

dachengx commented Aug 4, 2023 •

edited

Loading

dachengx commented Aug 10, 2023 •

edited

Loading