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.

Sign up for GitHub

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

Allow LogParabola to be defined using natural log #295

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Allow LogParabola to be defined using natural log #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
820672e
Allow LogParabola to be define using natural log
HealthyPear Jan 20, 2025
a1029eb
Add unit test for spectral.LogParabola
HealthyPear Jan 20, 2025
bffea3d
Add news fragment for PR #295
HealthyPear Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/changes/295.api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow a LogParabola spectral model to be initialized with a logarithm in base 10 or a natural logarithm using the boolean attribute `from_log10`.
13 changes: 10 additions & 3 deletions pyirf/spectral.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,25 +195,32 @@ class LogParabola:
:math:`\beta`
e_ref: astropy.units.Quantity[energy]
:math:`E_\text{ref}`
from_log10: bool
If True, compute the energy ration in the exponent factor
using base 10, else use natural logarithm.
Copy link
Member

@morcuended morcuended Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe here say that the default is base 10 log

Copy link
Member

Choose a reason for hiding this comment

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

I don't particularly like the keyword name here, why not call this "log_base" or just pass the log function?

E.g. log=np.log10 vs. log=np.log.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think you took the name from https://docs.gammapy.org/0.19/api/gammapy.modeling.models.LogParabolaSpectralModel.html#gammapy.modeling.models.LogParabolaSpectralModel.from_log10

But this is not a keyword, it's an alternative constructor, that's why it starts with from_.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the idea was to keep a nomenclature analog to gammapy which is what people tends to use more lately

Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't make sense to use the name of a class method for the name of a keyword

Copy link
Member

@maxnoe maxnoe Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You could implement the same (or rather the inverse) logic as gammapy, e.g. offer a from_ln method that converts the parameters to base 10.

Or you change the name to log, maybe with an enum like this:

class LogType(Enum):
    LOG10 = auto()
    LN = auto()

class LogParabola:
    def __init__(self, ..., log=LogType.LOG10)

"""

@u.quantity_input(
normalization=[DIFFUSE_FLUX_UNIT, POINT_SOURCE_FLUX_UNIT], e_ref=u.TeV
)
def __init__(self, normalization, a, b, e_ref=1 * u.TeV):
def __init__(self, normalization, a, b, e_ref=1 * u.TeV, from_log10=True):
"""Create a new LogParabola spectrum"""
self.normalization = normalization
self.a = a
self.b = b
self.e_ref = e_ref
self.from_log10 = from_log10

@u.quantity_input(energy=u.TeV)
def __call__(self, energy):
e = (energy / self.e_ref).to_value(u.one)
return self.normalization * e ** (self.a + self.b * np.log10(e))
log_factor = np.log10(e) if self.from_log10 else np.log(e)
return self.normalization * e ** (self.a + self.b * log_factor)

def __repr__(self):
return f"{self.__class__.__name__}({self.normalization} * (E / {self.e_ref})**({self.a} + {self.b} * log10(E / {self.e_ref}))"
log_factor = "log10" if self.from_log10 else "ln"
str_rep = f"{self.__class__.__name__}({self.normalization} * (E / {self.e_ref})**({self.a} + {self.b} * {log_factor}(E / {self.e_ref}))"
return str_rep


class PowerLawWithExponentialGaussian(PowerLaw):
Expand Down
57 changes: 57 additions & 0 deletions pyirf/tests/test_spectral.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,63 @@ def test_powerlaw():
assert power_law(1 * u.TeV).unit == unit
assert power_law(1 * u.GeV).unit == unit

def test_logparabola():
from pyirf.spectral import LogParabola

# Test initialization
normalization = 1e-12 * u.Unit("cm-2 s-1 TeV-1")
a = -2.0
b = 0.1
e_ref = 1 * u.TeV
lp = LogParabola(normalization, a, b, e_ref, from_log10=True)

assert lp.normalization == normalization
assert lp.a == a
assert lp.b == b
assert lp.e_ref == e_ref
assert lp.from_log10 is True

# Test the flux evaluation at a given energy
normalization = 1e-12 * u.Unit("cm-2 s-1 TeV-1")
a = -2.0
b = 0.1
e_ref = 1 * u.TeV
lp = LogParabola(normalization, a, b, e_ref, from_log10=True)

energy = 2 * u.TeV
expected_flux = normalization * (energy / e_ref) ** (
a + b * np.log10(energy / e_ref)
)
calculated_flux = lp(energy)

assert u.isclose(
calculated_flux, expected_flux, atol=1e-20 * u.Unit("cm-2 s-1 TeV-1")
)

# Same but for natural log
normalization = 1e-12 * u.Unit("cm-2 s-1 TeV-1")
a = -2.0
b = 0.1
e_ref = 1 * u.TeV
lp = LogParabola(normalization, a, b, e_ref, from_log10=False)

energy = 2 * u.TeV
expected_flux = normalization * (energy / e_ref) ** (a + b * np.log(energy / e_ref))
calculated_flux = lp(energy)

assert u.isclose(
calculated_flux, expected_flux, atol=1e-20 * u.Unit("cm-2 s-1 TeV-1")
)

# Test string representation
normalization = 1e-12 * u.Unit("cm-2 s-1 TeV-1")
a = -2.0
b = 0.1
e_ref = 1 * u.TeV
lp = LogParabola(normalization, a, b, e_ref, from_log10=True)

expected_repr = "LogParabola(1e-12 1 / (TeV s cm2) * (E / 1.0 TeV)**(-2.0 + 0.1 * log10(E / 1.0 TeV))"
assert repr(lp) == expected_repr

def test_powerlaw_from_simulations():
from pyirf.simulations import SimulatedEventsInfo
Expand Down
Loading