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 spectral table model #733

Merged
merged 3 commits into from Oct 25, 2016

Conversation

Projects
None yet
3 participants
@jjlk
Contributor

jjlk commented Oct 4, 2016

The purpose of this PR is to implement a spectral class initiated with energy and flux values (array). The class implementation follows closely what has been done in naima.

@cdeil cdeil added the feature label Oct 4, 2016

@cdeil cdeil changed the title from Add spectral table model class, TableModel to Add spectral table model Oct 4, 2016

@cdeil cdeil added this to the 0.5 milestone Oct 4, 2016

@cdeil cdeil self-assigned this Oct 4, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 4, 2016

Member

Thanks!
Let me know when I should have a look.
Please ignore the travis-ci fails for now, they are unrelated (other recent changes in Gammapy and Sherpa).

Member

cdeil commented Oct 4, 2016

Thanks!
Let me know when I should have a look.
Please ignore the travis-ci fails for now, they are unrelated (other recent changes in Gammapy and Sherpa).

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Oct 4, 2016

Contributor

Hi @cdeil,
Ready for a review!
I added a test for the TableModel class. I generate energy and flux points according to a power law (with parameters took from the PowerLaw test) and I verified that the values are the same (hope it's not machine dependant). I tried to pass the function table_model() as a pytest.fixture but failed. Thanks!

Contributor

jjlk commented Oct 4, 2016

Hi @cdeil,
Ready for a review!
I added a test for the TableModel class. I generate energy and flux points according to a power law (with parameters took from the PowerLaw test) and I verified that the values are the same (hope it's not machine dependant). I tried to pass the function table_model() as a pytest.fixture but failed. Thanks!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 4, 2016

Member

Looks good to me.
Personally I would have put the scipy import where it's used and not the initialiser, but I don't see any big advantage either way.

@joleroi - I'm assigning to you for final review.

Eventually we also want the translation to Sherpa table model so that it can be fit easily and a test that this works. But maybe just merge this now and then when someone has time to do this (and the current Sherpa issues on travis-ci are sorted out) we add it?

Member

cdeil commented Oct 4, 2016

Looks good to me.
Personally I would have put the scipy import where it's used and not the initialiser, but I don't see any big advantage either way.

@joleroi - I'm assigning to you for final review.

Eventually we also want the translation to Sherpa table model so that it can be fit easily and a test that this works. But maybe just merge this now and then when someone has time to do this (and the current Sherpa issues on travis-ci are sorted out) we add it?

@cdeil cdeil assigned joleroi and unassigned cdeil Oct 4, 2016

@cdeil

cdeil approved these changes Oct 4, 2016

@cdeil cdeil assigned cdeil and unassigned joleroi Oct 25, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 25, 2016

Member

travis-ci fails are unrelated:
https://travis-ci.org/gammapy/gammapy/jobs/170448989#L1729

Merging this now.

Member

cdeil commented Oct 25, 2016

travis-ci fails are unrelated:
https://travis-ci.org/gammapy/gammapy/jobs/170448989#L1729

Merging this now.

@cdeil cdeil merged commit 0586626 into gammapy:master Oct 25, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jjlk jjlk deleted the jjlk:table_model branch Oct 25, 2016

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