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.

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

Add tag on DM Annihilation spectral model #4071

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

registerrier
Copy link
Contributor

Description

This pull request solves #4056 by simply adding a tag on DarkMatterAnnihilationSpectralModel with short name dm-annihilation.
Note that DM spatial models are not usable in SkyModel as they are not regular SpatialModel.

Dear reviewer

@registerrier registerrier linked an issue Sep 9, 2022 that may be closed by this pull request
@registerrier registerrier added this to the 1.0 milestone Sep 9, 2022
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #4071 (12e96c0) into master (95dc464) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4071      +/-   ##
==========================================
- Coverage   94.04%   94.00%   -0.05%     
==========================================
  Files         162      162              
  Lines       21444    21447       +3     
==========================================
- Hits        20167    20161       -6     
- Misses       1277     1286       +9     
Impacted Files Coverage Δ
gammapy/astro/darkmatter/spectra.py 98.71% <100.00%> (+0.35%) ⬆️
gammapy/scripts/download.py 84.61% <0.00%> (-15.39%) ⬇️
gammapy/utils/scripts.py 90.47% <0.00%> (-5.18%) ⬇️
gammapy/modeling/models/temporal.py 97.07% <0.00%> (-1.99%) ⬇️
gammapy/maps/wcs/ndmap.py 90.90% <0.00%> (-0.22%) ⬇️
gammapy/catalog/hess.py 97.75% <0.00%> (ø)
gammapy/catalog/gammacat.py 90.75% <0.00%> (ø)
gammapy/maps/region/ndmap.py 93.37% <0.00%> (ø)
gammapy/visualization/utils.py 93.67% <0.00%> (ø)
gammapy/estimators/points/core.py 91.90% <0.00%> (ø)
... and 6 more

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

@adonath
Copy link
Member

adonath commented Sep 9, 2022

Thanks @registerrier! My main question is whether adding the tag really solves the problem? The DarkMatterAnnihilationSpectralModel as much more associated information , which needs to be serialized with a custom .to_dict() methods. Including k, z, mass, channel...

@adonath adonath self-assigned this Sep 9, 2022
@registerrier
Copy link
Contributor Author

Thanks @registerrier! My main question is whether adding the tag really solves the problem? The DarkMatterAnnihilationSpectralModel as much more associated information , which needs to be serialized with a custom .to_dict() methods. Including k, z, mass, channel...

This is a good point. Technically, some of these quantities could be Parameter objects. But not e.g. channel. How to serialize non Parameter attributes is not clear.
Maybe this is a similar case to NaimaSpectralModel which has no yaml serialization for now.

@adonath
Copy link
Member

adonath commented Sep 14, 2022

@registerrier Yes, maybe let's just raise a meaningful error for now?

Signed-off-by: Terrier <rterrier@apc.in2p3.fr>
Signed-off-by: Terrier <rterrier@apc.in2p3.fr>
@registerrier
Copy link
Contributor Author

I have included additional attributes in the out dictionary. It actually works and allows exporting DMmodels to yaml.

Obviously we will need to look at what should be a Parameter and what is a simple model attribute. But that could be left for later as the DM models will have to be better included in the Model framework (or they will have to be removed if those models are found to be useless).

Opinions @adonath ?

@adonath
Copy link
Member

adonath commented Sep 14, 2022

Thanks @registerrier, great to see it works with limited effort. I'll make a more thorough review later...

Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks @registerrier, I have left one minor comment and one inline suggestion...

Comment on lines 215 to 225
def from_dict(cls, data):
data = data["spectral"]

channel = data["channel"]
mass = u.Quantity(data["mass"])
jfactor = u.Quantity(data["jfactor"])
z = data["z"]
k = data["k"]

scale = [p["value"] for p in data["parameters"] if p["name"] == "scale"][0]
return cls(mass, channel, scale=scale, jfactor=jfactor, z=z, k=k)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def from_dict(cls, data):
data = data["spectral"]
channel = data["channel"]
mass = u.Quantity(data["mass"])
jfactor = u.Quantity(data["jfactor"])
z = data["z"]
k = data["k"]
scale = [p["value"] for p in data["parameters"] if p["name"] == "scale"][0]
return cls(mass, channel, scale=scale, jfactor=jfactor, z=z, k=k)
def from_dict(cls, data):
"""Create spectral model from dict
Parameters
----------
data : dict
DIct with model data
Returns
-------
model : `DarkMatterAnnihilationSpectralModel`
Dark matter annihilation spectral model
"""
data = data["spectral"]
scale = [p["value"] for p in data.pop("parameters") if p["name"] == "scale"][0]
return cls(scale=scale, **data)


def __init__(self, mass, channel, scale=scale.quantity, jfactor=1, z=0, k=2):
self.k = k
self.z = z
self.mass = mass
self.mass = u.Quantity(mass)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a unit test to establish the quantity here...

@adonath
Copy link
Member

adonath commented Sep 20, 2022

What is the status here @registerrier ?

Signed-off-by: Terrier <rterrier@apc.in2p3.fr>
Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks @registerrier, no further comments from my side.

@adonath adonath merged commit d5ca181 into gammapy:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't write DarkMatter Model to file
2 participants