Add interp_scale to parameter scaling #4218

QRemy · 2022-11-25T11:23:25Z

Use the interpolation scale given by Parameter.interp in the transform from the parameter value to the factor seen by the optimizer, as proposed in #4027.
Not sure how to combine this with the scale method, for now I apply interp_scale first and then the scale method.

adonath · 2022-12-09T17:01:14Z

Thanks @QRemy! Should deprecate the "old scaling" methods? @registerrier

QRemy · 2022-12-09T17:47:43Z

For now they are still applied after the scaling from the .interp argument, not sure if it makes sense. I would prefer to keep them even if they are not applied in a row but alternatively, so we can still try to reproduce previous results.

adonath · 2023-01-25T01:42:59Z

@QRemy I think we never really discussed this PR in our dev call, would you mind setting it on the agenda for this Friday? https://github.com/gammapy/gammapy-meetings/tree/master/dev-meetings/2023/2023-01-27 Thanks!

QRemy · 2023-02-08T16:18:22Z

For backward compatibility I added a scale_interp="lin" argument instead of using direclty interp so the transform is done correctly for previously written models.

codecov · 2023-02-08T16:29:27Z

Codecov Report

Attention: Patch coverage is 97.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 94.82%. Comparing base (02c45f9) to head (34ed762).
Report is 874 commits behind head on main.

❗ Current head 34ed762 differs from pull request most recent head 34ced3a

Please upload reports for the commit 34ced3a to get more accurate results.

Files	Patch %	Lines
gammapy/modeling/parameter.py	97.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #4218       +/-   ##
===========================================
+ Coverage   75.71%   94.82%   +19.10%     
===========================================
  Files         228      215       -13     
  Lines       33778    30361     -3417     
===========================================
+ Hits        25576    28790     +3214     
+ Misses       8202     1571     -6631

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gammapy/modeling/parameter.py

adonath · 2023-04-28T14:03:37Z

Two things:

e.g. log scaling is and implicit prior and we are not sure how this relates to PIG 16 - Model Priors API #4381
we would need propagation of error through the transform via transform of random variables...

bkhelifi · 2023-09-26T15:06:46Z

@QRemy what is the status of this work?

gammapy/modeling/parameter.py

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

Astro-Kirsty · 2024-05-07T08:14:42Z

@QRemy, what is the status here?
Once rebased, ready for review?

QRemy · 2024-05-07T12:43:51Z

The error propagation for the transform is still missing.

bkhelifi

Thanks a lot @QRemy. It seems more coherent now. I have just put some suggestions on the docstrings

bkhelifi · 2024-05-22T08:08:33Z

gammapy/modeling/parameter.py

@@ -97,6 +97,8 @@ class Parameter:
        Whether the parameter represents the flux norm of the model.
    prior : `~gammapy.modeling.models.Prior`
        Prior set on the parameter.
+    scale_interp : {"lin", "sqrt", "log"}


Can you profit of this PR to add in the docstring all the default values? Thanks

gammapy/modeling/parameter.py

bkhelifi · 2024-05-22T08:12:03Z

gammapy/modeling/parameter.py

-            self.factor, self.scale = 1, self.value
+            self._scale = value
+
+    def transform(self, value, update_scale=False):


What does exactly this function? Do not forget the default value of the argument

bkhelifi · 2024-05-22T08:12:22Z

gammapy/modeling/parameter.py

+            self.update_scale(factor)
+        return factor / self.scale
+
+    def inverse_transform(self, factor):


What does exactly this function? A docstring might be great

gammapy/modeling/parameter.py

bkhelifi · 2024-05-22T08:15:12Z

gammapy/modeling/parameter.py

@@ -824,12 +859,16 @@ def __init__(
        min=np.nan,
        max=np.nan,
        error=0,
+        scale_method="scale10",


AS this class is exposed, it might be great to add a docstring describing it with its parameters (and default values)

Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>

QRemy · 2024-05-22T11:23:41Z

The error propagation for the transform is still missing.

I will try a solution based on
https://hdembinski.github.io/jacobi/reference.html#jacobi.propagate
There is a discussion about integrating it in scipy so we may be able to get rid of this extra dependency later.

QRemy requested review from adonath and registerrier November 25, 2022 11:23

QRemy added the feature label Nov 25, 2022

QRemy marked this pull request as ready for review December 8, 2022 15:01

QRemy force-pushed the scale_transform branch from 2f7d4eb to cf9e393 Compare December 9, 2022 08:59

QRemy force-pushed the scale_transform branch from cf9e393 to 037fc9e Compare February 8, 2023 16:12

QRemy commented Feb 10, 2023

View reviewed changes

gammapy/modeling/parameter.py Outdated Show resolved Hide resolved

QRemy force-pushed the scale_transform branch from 3bc58ef to 34ed762 Compare February 21, 2023 16:25

registerrier added this to the 1.1 milestone Apr 21, 2023

adonath modified the milestones: 1.1, 1.2 Apr 28, 2023

QRemy commented Dec 20, 2023

View reviewed changes

gammapy/modeling/parameter.py Outdated Show resolved Hide resolved

QRemy added 5 commits December 20, 2023 18:04

add interp_scale to parameter scaling

883839b

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

minor style edit

c28dce4

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

add scale_interp argument for backward compatibility

f2e89a2

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

setter to change scale_method and scale_interp properly

47fcb3f

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

fix io

b9b8b27

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

QRemy force-pushed the scale_transform branch from 34ed762 to b9b8b27 Compare December 20, 2023 17:05

QRemy and others added 2 commits December 20, 2023 18:05

Update gammapy/modeling/parameter.py

96a59ae

fix after rebase

1da4c7f

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

registerrier modified the milestones: 1.2, 1.3 Feb 2, 2024

bkhelifi requested changes May 22, 2024

View reviewed changes

Apply suggestions from code review

34ced3a

Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interp_scale to parameter scaling #4218

Add interp_scale to parameter scaling #4218

QRemy commented Nov 25, 2022

adonath commented Dec 9, 2022

QRemy commented Dec 9, 2022 •

edited

adonath commented Jan 25, 2023

QRemy commented Feb 8, 2023

codecov bot commented Feb 8, 2023 •

edited

adonath commented Apr 28, 2023

bkhelifi commented Sep 26, 2023

Astro-Kirsty commented May 7, 2024

QRemy commented May 7, 2024

bkhelifi left a comment

bkhelifi May 22, 2024

bkhelifi May 22, 2024

bkhelifi May 22, 2024

bkhelifi May 22, 2024

QRemy commented May 22, 2024

Add interp_scale to parameter scaling #4218

Are you sure you want to change the base?

Add interp_scale to parameter scaling #4218

Conversation

QRemy commented Nov 25, 2022

adonath commented Dec 9, 2022

QRemy commented Dec 9, 2022 • edited

adonath commented Jan 25, 2023

QRemy commented Feb 8, 2023

codecov bot commented Feb 8, 2023 • edited

Codecov Report

adonath commented Apr 28, 2023

bkhelifi commented Sep 26, 2023

Astro-Kirsty commented May 7, 2024

QRemy commented May 7, 2024

bkhelifi left a comment

Choose a reason for hiding this comment

bkhelifi May 22, 2024

Choose a reason for hiding this comment

bkhelifi May 22, 2024

Choose a reason for hiding this comment

bkhelifi May 22, 2024

Choose a reason for hiding this comment

bkhelifi May 22, 2024

Choose a reason for hiding this comment

QRemy commented May 22, 2024

QRemy commented Dec 9, 2022 •

edited

codecov bot commented Feb 8, 2023 •

edited