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 XSPEC table absorption model to spectral table model #824

Merged
merged 3 commits into from Dec 19, 2016

Conversation

@jjlk
Copy link
Contributor

@jjlk jjlk commented Dec 18, 2016

Hi @cdeil and @joleroi ,
I added a class method to TableModel to import models from XSPEC (see flx2tab).

Maybe we could propose this format on https://gamma-astro-data-formats.readthedocs.io/en/latest/? @cdeil, @registerrier , some volunteers?

I also made optional the logarithm scale value on the y-axis and I added a plot function because, as I understood it, the SpectralModel plot method was really attached to have some physical units for the y-values.

Thanks!

@cdeil cdeil added the feature label Dec 19, 2016
@cdeil cdeil added this to the 0.6 milestone Dec 19, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Dec 19, 2016

Please add $GAMMAPY_EXTRA/datasets/ebl/ebl_franceschini.fits.gz so that tests run:
https://travis-ci.org/gammapy/gammapy/jobs/184942465#L997

You can directly add the file to master in gammapy-extra, unless you want it reviewed.
The usual things are: please add a README file what it is in the folder, and don't commit too large files without discussion (ask first if it's >> 1 MB).

Concerning format -- yes, I'd pretty much always would prefer formats are described over at gamma-astro-data-formats, instead of in the Gammapy docs. Especially for things like this, where the same data is of interest to several science tools.
I don't have time for this one. If you do, the start would be to either make a pull request with a very short and simple description of what you have here (as a starting point) or an issue proposing it, and CC a few people that might be able to give useful feedback.

Copy link
Member

@cdeil cdeil left a comment

I left a few inline comments.

filename = str(make_path(filename))

# Check if parameter value is in range
table_param = fits.open(filename)['PARAMETERS']

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

Can you try and use astropy.table.Table here?
We use it almost everywhere else instead of astropy.io.fits.BinTable directly.
To read a specific HDU:

Table.read(filename, hdu='MY_HDU_NAME')
filename = '$GAMMAPY_EXTRA/datasets/ebl/ebl_franceschini.fits.gz'
table_model = TableModel.from_file(filename=filename, param=0.3)
"""
energy_col = 'ENERG'

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

You're using this energy_col = 'ENERG' two times below, with string formatting.
IMO it would be simpler to remove it, and just use the column names directly below.

Examples
--------
Fill table from a model

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

To get proper syntax highlighting, you have to indent the code in the docstring by four spaces, like this:

Here's some code:

    print('hello!')
fill_value=-np.Inf,
bounds_error=False,
kind='cubic')

@classmethod
def from_file(cls, filename, param):

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

What is param???

Mention it with one line (or add a Parameters section) in the docstring?

…eter y_label in plot function
@jjlk
Copy link
Contributor Author

@jjlk jjlk commented Dec 19, 2016

Hi @cdeil ,

Please add $GAMMAPY_EXTRA/datasets/ebl/ebl_franceschini.fits.gz so that tests run:
https://travis-ci.org/gammapy/gammapy/jobs/184942465#L997

The files are already there. The test failed on Travis because of a precision problem. Locally it works fine for me. For now I just changed the parameter value for the test.

Concerning format -- yes, I'd pretty much always would prefer formats are described over at gamma-astro-data-formats, instead of in the Gammapy docs. Especially for things like this, where the same data is of interest to several science tools.
I don't have time for this one. If you do, the start would be to either make a pull request with a very short and simple description of what you have here (as a starting point) or an issue proposing it,
and CC a few people that might be able to give useful feedback.

Oké, I'll have a look at that later.

Thanks for your comments, I took all of them into account!

@joleroi
Copy link
Contributor

@joleroi joleroi commented Dec 19, 2016

I added a plot function because, as I understood it, the SpectralModel plot method was really attached to have some physical units for the y-values.

Doesn't it work if you pass flux_unit=''?

I had a discussion with @adonath about this and we decided it might be better do rename flux_unit -> y_unit everywhere. Its just not top priority I think, but if you want to do it, go ahead (would also need to be done in gammapy/spectrum/results.py)

Copy link
Contributor

@joleroi joleroi left a comment

Minor stuff

kind='cubic')
@classmethod
def from_file(cls, filename, param):
"""A Table containing aborbed values from a XSPEC model

This comment has been minimized.

@joleroi

joleroi Dec 19, 2016
Contributor

typo: absorbed (?)

table_param = Table.read(filename, hdu='PARAMETERS')
param_min = table_param['MINIMUM']
param_max = table_param['MAXIMUM']
assert(param >= param_min and param <= param_max), "Parameter\

This comment has been minimized.

@joleroi

joleroi Dec 19, 2016
Contributor

I've never seen a hard line break within a string literal coded like this, So I don't know if it's "allowed". What I've seen many time is

a = " Very long string, It's so long that we need a new "
      "line somewhere"

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

A coding rule for Gammapy (and Astropy / ... most projects): Never use assert to check inputs, raise an exception instead.

Here:

if param_min <= param <= param_max:
    raise ValueError(<message here>)
energy = energy_bounds.log_centers

# Get spectrum values (no interpolation, take closest value for param)
table_spectra = Table.read(filename, hdu='SPECTRA')

This comment has been minimized.

@joleroi

joleroi Dec 19, 2016
Contributor

Is it ok to hardcode the hdu names? Is this always the same for the format you're using, or is it better to pass the hdu names as kwargs?

kind='cubic')
@classmethod
def from_file(cls, filename, param):
"""A Table containing aborbed values from a XSPEC model

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

Maybe call the method read_<something> instead of from_file?
E.g. read_xspec_absorbed or read_ebl?

I think we always use the term "read" for classmethods reading from files, or at least I'm +1 to do that.

Copy link
Member

@cdeil cdeil left a comment

Left two minor inline comments.

File containing the XSPEC model
param : float
Model parameter value
Examples

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

I don't think this docstring will render correctly.
You need to put an empty line before the Examples heading, and I think also colon before the code block.

Can you please check locally with

python setup.py build_docs
open docs/_build/html/index.html

and then check this docstring on the HGPS page?

We will add automated checks for docstrings in the future, there's a discussion for Astropy generally how to best do it. For now, checking docstrings is done manually.

@requires_data('gammapy-extra')
def test_table_model_from_file():
filename = '$GAMMAPY_EXTRA/datasets/ebl/ebl_franceschini.fits.gz'
energy_range = (0.03, 10)

This comment has been minimized.

@cdeil

cdeil Dec 19, 2016
Member

No need for local variables here IMO, since only used once.
Just pass as keyword arguments below.

@jjlk
Copy link
Contributor Author

@jjlk jjlk commented Dec 19, 2016

Hi @joleroi and @cdeil ,

Doesn't it work if you pass flux_unit=''?
I had a discussion with @adonath about this and we decided it might be better do rename flux_unit -> y_unit everywhere. Its just not top priority I think, but if you want to do it, go ahead (would also need to be done in gammapy/spectrum/results.py)

I tried that and it didn't work. I didn't push a lot to investiguate where the problem was.

Maybe call the method read_ instead of from_file?
E.g. read_xspec_absorbed or read_ebl?

Done, read_xspec_model

I don't think this docstring will render correctly.

Fixed

A coding rule for Gammapy (and Astropy / ... most projects): Never use assert to check inputs, raise an exception instead.

Fixed

Is it ok to hardcode the hdu names? Is this always the same for the format you're using, or is it better to pass the hdu names as kwargs?

Yes, it is a standard

No need for local variables here IMO, since only used once.
Just pass as keyword arguments below.

Fixed

And multiple typos fixed.

Thanks!

@cdeil
cdeil approved these changes Dec 19, 2016
Copy link
Member

@cdeil cdeil left a comment

Looks good to me.

@joleroi - Do you want to do a final review, or should we merge?

(I'd suggest postponing plot y-axis clean-up to a separate issue / PR)

@joleroi joleroi merged commit 1fec65b into gammapy:master Dec 19, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jjlk jjlk deleted the jjlk:xspec_model branch Dec 19, 2016
@cdeil cdeil changed the title Modify TableModel to handle absorption model from XSPEC table Add XSPEC table absorption model to spectral table model Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants