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 XML SkyModel serialization #1397

Merged
merged 5 commits into from Apr 27, 2018
Merged

Add XML SkyModel serialization #1397

merged 5 commits into from Apr 27, 2018

Conversation

@joleroi
Copy link
Contributor

@joleroi joleroi commented Apr 25, 2018

This PR adds support for XML serialisation of sky models.

@joleroi joleroi requested a review from cdeil Apr 25, 2018
@joleroi joleroi changed the title Introducing gammapy.utils.serialization WIP: Introducing gammapy.utils.serialization Apr 25, 2018
@cdeil cdeil self-assigned this Apr 25, 2018
@cdeil cdeil added this to the 0.8 milestone Apr 25, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Apr 25, 2018

@joleroi - Thanks!

I think my preference would be to do this:

from gammapy.cube.models import SkyModels
models = SkyModels.read('models.xml')

and basically have this:

class SkyModels:
    def __init__(self, model_list):
        self._model_list = model_list

It's close to what you put here, but it

  • wraps a list instead of subclassing list - that means we can put any API we like, and start by exposing a minimal API
  • exposes the I/O in a class / place that's nice to find for users - read would just call into gammapy.utils, but I think for users it's a bit nicer to have that factory method accessible with the models.
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Apr 26, 2018

I'm -1 on SkyModels to close to SkyModel will confuse everyone, no? It's analog to Observation and ObservationList, isn't it? So I'd like to stick with SkyModelList. I'm fine with having a list as data member not subclassing it.

@cdeil
Copy link
Member

@cdeil cdeil commented Apr 26, 2018

We've had these discussions before. "List" shouldn't be in the name if it's not a list, but a wrapper that doesn't really expose the list API (like slicing, ...).
I could dig up issues that result from subclassing, e.g. HDUList is a list subclass, but then some list methods don't work properly, e.g. .copy() gives crazy results.
There's a lot of things you have to think about and take care of when you make a list, and when callers start using the list API you're bound to it.
In that sense, exposing a minimal API is the safe way to start, and then it's easier to change to something else later.

My preference would be SkyModels and Observations for the containers, but I'm also OK with SkyModelCollection or SkyModelLibrary or something like that if you prefer.

@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Apr 26, 2018

No need to argue about not subclassing list, I already agreed 👍

We've had these discussions before. "List" shouldn't be in the name if it's not a list, but a wrapper that doesn't really expose the list API (like slicing, ...).

Also agreed, I will thing about a name, and fall back to SkyModels if I cant't come up with anything

@joleroi joleroi force-pushed the joleroi:xml_serialization branch from a1c353e to f496038 Apr 26, 2018
@joleroi joleroi changed the title WIP: Introducing gammapy.utils.serialization Introducing gammapy.utils.serialization Apr 26, 2018
<parameter free="0" max="1" min="-1" name="Index" scale="1.0" value="0"></parameter>
<parameter free="0" max="2e2" min="5e1" name="Scale" scale="1.0" value="1e2"></parameter>
</spectrum>
<spatialModel file="$GAMMAPY_EXTRA/datasets/fermi_iem/gll_iem_v02.fit" type="MapCubeFunction">

This comment has been minimized.

@joleroi

joleroi Apr 26, 2018
Author Contributor

I didn't push anything to gammapy-extra yet. This old version of the diffuse model is ~30 MB. OK to add? Or do you have a better idea for a test case?

This comment has been minimized.

@cdeil

cdeil Apr 26, 2018
Member

There is e.g. gammapy-extra/test_datasets/unbundled/fermi/gll_iem_v02_cutout.fits.
Let's not add a 30 MB file.
IMO even better would be to not use data files if not really needed, and instead create the file in a temp folder in the text with a few lines.
With gammapy.maps it's now easy to make any kind of map; for example:
https://github.com/gammapy/gammapy/blob/master/gammapy/cube/tests/test_models.py#L14

This comment has been minimized.

@joleroi

joleroi Apr 27, 2018
Author Contributor

👍

sourcelib = SourceLibrary.from_xml(filename)
assert len(sourcelib.skymodels) == 4

# TODO: add more tests

This comment has been minimized.

@joleroi

joleroi Apr 26, 2018
Author Contributor

This will be part of this PR

@joleroi joleroi force-pushed the joleroi:xml_serialization branch from f496038 to c395410 Apr 27, 2018
@joleroi joleroi merged commit 42416f5 into gammapy:master Apr 27, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@joleroi joleroi deleted the joleroi:xml_serialization branch Apr 27, 2018
@joleroi joleroi changed the title Introducing gammapy.utils.serialization Add XML SkyModel serialization Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants