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 new flux point class #810

Merged
merged 10 commits into from Dec 8, 2016

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Dec 7, 2016

This PR adds a prototype for the FluxPoints class, that implements the data specification defined by
http://gamma-astro-data-formats.readthedocs.io/en/latest/results/flux_points/index.html#flux-points

So far it mainly handles validation of the input tables and plotting of flux points with errors and upper limits. I'll make a second PR to adapt gammapy.catalog to use this class.

@cdeil @joleroi @woodmd

@adonath adonath added the feature label Dec 7, 2016

@adonath adonath added this to the 0.6 milestone Dec 7, 2016

@adonath adonath self-assigned this Dec 7, 2016

@cdeil

Looks good to me.
Maybe let's continue with this now and re-discuss whether to introduce a separate class for int fluxes later?

How much extra work would it be to remove DifferentialFluxPoints here?
I think we should do this ASAP, so that it's clear what code to work on and start adapting other scripts.

def test_plot(self, flux_points):
import matplotlib.pyplot as plt
ax = plt.gca()
flux_points.plot(ax=ax)

This comment has been minimized.

@cdeil

cdeil Dec 7, 2016

Member

All plotting functions should do

if ax is None:
    ax = plt.gca()

at the start, so no need to create an ax and pass it from the test.

@cdeil

cdeil Dec 7, 2016

Member

All plotting functions should do

if ax is None:
    ax = plt.gca()

at the start, so no need to create an ax and pass it from the test.

This comment has been minimized.

@adonath

adonath Dec 7, 2016

Member

Done

@adonath

adonath Dec 7, 2016

Member

Done

def test_e_ref(self, flux_points):
actual = flux_points.e_ref
if flux_points.sed_type == 'dnde':

This comment has been minimized.

@cdeil

cdeil Dec 7, 2016

Member

I guess all of these if / else aren't ideal.
Maybe at least two test classes to get rid of this would make sense?
(not sure)

@cdeil

cdeil Dec 7, 2016

Member

I guess all of these if / else aren't ideal.
Maybe at least two test classes to get rid of this would make sense?
(not sure)

This comment has been minimized.

@adonath

adonath Dec 7, 2016

Member

For now I prefer to keep it because I don't want to duplicate the testing code.

@adonath

adonath Dec 7, 2016

Member

For now I prefer to keep it because I don't want to duplicate the testing code.

info += "Flux points of type '{}'".format(self.sed_type)
return info
def info(self):

This comment has been minimized.

@cdeil

cdeil Dec 7, 2016

Member

Suggest to remove info and just keep __str__.
This is what we've done for most classes, at least for now.

@cdeil

cdeil Dec 7, 2016

Member

Suggest to remove info and just keep __str__.
This is what we've done for most classes, at least for now.

Show outdated Hide outdated gammapy/spectrum/flux_point.py Outdated
Filename
kwargs : dict
Keyword arguments passed to `~astropy.table.Table.write`.
"""

This comment has been minimized.

@cdeil

cdeil Dec 7, 2016

Member

I suggest to also pass filename through:

filename = make_path(filename)

It does e.g. os environ variable expansion, which is something I find convenient and use frequently.

@cdeil

cdeil Dec 7, 2016

Member

I suggest to also pass filename through:

filename = make_path(filename)

It does e.g. os environ variable expansion, which is something I find convenient and use frequently.

This comment has been minimized.

@adonath

adonath Dec 7, 2016

Member

Done

@adonath

adonath Dec 7, 2016

Member

Done

Show outdated Hide outdated gammapy/spectrum/flux_point.py Outdated

@cdeil cdeil referenced this pull request Dec 7, 2016

Merged

Add E^2 dnde to SED format? #77

adonath added some commits Dec 7, 2016

@adonath adonath merged commit 81a5910 into gammapy:master Dec 8, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 8, 2016

Member

🎉

Member

cdeil commented Dec 8, 2016

🎉

@cdeil cdeil changed the title from Add flux point class to Add new flux point class Dec 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment