Implement FitStatistics / Priors ideas. #4237

nbiederbeck · 2022-12-06T16:07:38Z

This is a very preliminary proposal for implementing (1) more flexible FitStatistics, and (2) priors on Parameters.

With this implementation, both can have the same base class, are equally serializable, can have non-breaking defaults, and can be provided by users.

Dear reviewer

Certainly I am missing something obvious. I open this PR, so we can look at the same code at the same time.

Please see also my slides from the coding sprint and the earlier notebook from Axel

registerrier

Thanks @nbiederbeck .

I have left inline comments. Maybe adding docstrings would help.

My main comment is that FitStatistic should be independent of the Dataset structure but only expect array-like inputs.

registerrier · 2022-12-07T07:43:25Z

gammapy/modeling/models/core.py

@@ -163,6 +163,9 @@ def parameters(self):
            [getattr(self, name) for name in self.default_parameters.names]
        )

+    def stat_sum(self):


It seems to me that the stat_sum should rather be on the Models or Parameters object. That's where we have the global information on the models used.
Is there a use case for computing the priors associated to a single model?

Note that priors might inherit from ModelBase. In which case the evaluate or __call__ methods would be used to compute the associated statistic

I think you are right. To me it seems that a single model is a subset of Models (e.g. len(Models) == 1) right? Then this would surely be better placed there. Please forgive my ignorance about the Gammapy internals and where put this best.

I think we should figure out a clean API here. I think the Models.stat_sum() is confusing a bit as well. Long-term we would like to have a more separated API between MapDataset, Models and Fit. I am very sure that we will evolve into a direction where models are passed to e.g MapDataset.stat_sum(models=models). So I would propose to evolve into a direction of e.g. implementing a PriorFitStatistic class, which takes the Models class and evaluates the priors. Like PriorFitStatistic(beta=1.).stat_sum(models=models). By now it seems to me this PriorFitStatistics will only need to be supported for Datasets.

registerrier · 2022-12-07T07:45:58Z

gammapy/modeling/parameter.py

@@ -145,6 +147,12 @@ def __init__(
        self.interp = interp
        self.scale_method = scale_method

+    def stat_sum(self):


This does very little in practice. It is probably not needed. Everything could calculated in the global stat_sum where the loop over parameters is performed.

registerrier · 2022-12-07T07:51:24Z

gammapy/datasets/map.py

+class FitStatistic:
+    """Calculate -2 * log(L)."""
+
+    def stat_sum(self, dataset):


I think it does not make sense to bind FitStatistic to the Dataset classes. They should only expect array-like input. (see CountsStatistic).

Yes, you are totally right about that!

registerrier · 2022-12-07T07:55:48Z

gammapy/datasets/map.py

@@ -180,6 +200,7 @@ class MapDataset(Dataset):
    psf = LazyFitsData(cache=True)
    mask_fit = LazyFitsData(cache=True)
    mask_safe = LazyFitsData(cache=True)
+    fit_statistic = CashFitStatistic()


This will likely require a setter method because we will have to check that a FitStatistic is applicable to a Dataset.

nbiederbeck · 2022-12-07T12:43:25Z

gammapy/modeling/parameter.py

+            if par.prior is not None:
+                args = {}
+                # this is almost duplicated code from gammapy.dataset.map
+                for key in signature(par.prior.stat_sum).parameters.keys():


I think currently this might evaluate all Priors for all Parameters. This would of course has to be fixed properly. I was thinking about

parameters.x # but need to do parameters['x'].value

and so each parameter has the attribute .value.

nbiederbeck · 2022-12-07T12:44:03Z

gammapy/modeling/tests/test_fit.py

@@ -319,3 +322,41 @@ def test_stat_contour():

    # Check that original value state wasn't changed
    assert_allclose(dataset.models.parameters["y"].value, 300)
+
+
+def test_with_prior():


This test is certainly at the wrong position, but I added it to show how setting and evaluating priors can work.

nbiederbeck · 2022-12-07T12:52:15Z

While doing this, I was thinking about how to implement #3955. I need to further investigate this, but I assume it is going in a good direction. Please let me know what you think. Thank you.

codecov · 2022-12-07T13:02:46Z

Codecov Report

Merging #4237 (1032771) into main (ec9df57) will decrease coverage by 0.12%.
The diff coverage is 61.68%.

@@            Coverage Diff             @@
##             main    #4237      +/-   ##
==========================================
- Coverage   94.83%   94.71%   -0.12%     
==========================================
  Files         216      216              
  Lines       30501    30597      +96     
==========================================
+ Hits        28925    28980      +55     
- Misses       1576     1617      +41

Impacted Files	Coverage Δ
gammapy/irf/background.py	`97.00% <ø> (ø)`
gammapy/irf/edisp/map.py	`96.47% <ø> (ø)`
gammapy/modeling/models/core.py	`94.50% <35.71%> (-1.66%)`	⬇️
gammapy/datasets/map.py	`92.17% <59.61%> (-1.69%)`	⬇️
gammapy/modeling/parameter.py	`94.43% <66.66%> (-2.42%)`	⬇️
gammapy/data/data_store.py	`94.42% <100.00%> (ø)`
gammapy/datasets/core.py	`92.83% <100.00%> (+0.05%)`	⬆️
gammapy/irf/core.py	`93.11% <100.00%> (ø)`
gammapy/irf/io.py	`92.42% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

bkhelifi · 2023-02-27T17:09:58Z

gammapy/datasets/map.py

@@ -1080,13 +1179,18 @@ def plot_residuals(
        return ax_spatial, ax_spectral

    def stat_sum(self):
-        """Total statistic function value given the current model parameters."""
-        counts, npred = self.counts.data.astype(float), self.npred().data
+        """Total likelihood given the current model parameters."""


Is it really the likelihood or -2log(L)?

This is a very preliminary proposal for implementing (1) more flexible FitStatistics (2) priors on Parameters With this implementation, both can have the same base class, are equally serializable, can have non-breaking defaults, and be provided by users. Signed-off-by: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>

Signed-off-by: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>

bkhelifi · 2023-09-26T15:55:36Z

@nbiederbeck can you update your code of your branch from the main, such that the tests are passing? thanks

nbiederbeck · 2023-10-05T12:46:58Z

Hi @bkhelifi, thanks for reaching out. My plate is full this month, I can start looking into this in November. If someone steps in earlier and takes over, I'm happy to assist.

registerrier reviewed Dec 7, 2022

View reviewed changes

registerrier marked this pull request as draft December 7, 2022 08:02

nbiederbeck commented Dec 7, 2022

View reviewed changes

nbiederbeck force-pushed the fit-statistics-and-priors branch 2 times, most recently from e5d7932 to 6d23f1c Compare February 14, 2023 09:56

nbiederbeck force-pushed the fit-statistics-and-priors branch from 6d23f1c to ddb263a Compare February 22, 2023 15:37

bkhelifi reviewed Feb 27, 2023

View reviewed changes

nbiederbeck force-pushed the fit-statistics-and-priors branch from ddb263a to 6da8280 Compare March 10, 2023 08:03

nbiederbeck added 10 commits March 15, 2023 09:06

Fix tests where two models share a name

668ffa1

Signed-off-by: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>

Evaluate stat_sum for priors on Parameters

cd34665

Signed-off-by: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>

Make FitStatistic independent from Dataset

56bb6a2

Signed-off-by: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>

Add test: Evaluate Priors

4bbb869

Signed-off-by: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>

Remove unused import

0a6613b

Add priors to Parameters

fe0954b

evaluate model.prior

4df7060

add Tikhonov Prior

3778a74

Fix error

1032771

nbiederbeck force-pushed the fit-statistics-and-priors branch from 6da8280 to 1032771 Compare March 15, 2023 08:06

registerrier added this to the 1.2 milestone Apr 21, 2023

registerrier modified the milestones: 1.2, wishlist Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FitStatistics / Priors ideas. #4237

Implement FitStatistics / Priors ideas. #4237

nbiederbeck commented Dec 6, 2022

registerrier left a comment

registerrier Dec 7, 2022

registerrier Dec 7, 2022

nbiederbeck Dec 7, 2022

adonath Mar 15, 2023 •

edited

registerrier Dec 7, 2022

registerrier Dec 7, 2022

nbiederbeck Dec 7, 2022

registerrier Dec 7, 2022

nbiederbeck Dec 7, 2022

nbiederbeck Dec 7, 2022

nbiederbeck commented Dec 7, 2022

codecov bot commented Dec 7, 2022 •

edited

bkhelifi Feb 27, 2023

bkhelifi commented Sep 26, 2023

nbiederbeck commented Oct 5, 2023

Implement FitStatistics / Priors ideas. #4237

Are you sure you want to change the base?

Implement FitStatistics / Priors ideas. #4237

Conversation

nbiederbeck commented Dec 6, 2022

registerrier 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

adonath Mar 15, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbiederbeck commented Dec 7, 2022

codecov bot commented Dec 7, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

bkhelifi commented Sep 26, 2023

nbiederbeck commented Oct 5, 2023

adonath Mar 15, 2023 •

edited

codecov bot commented Dec 7, 2022 •

edited