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 AbsorbedSpectralModel and improve CTA IRF class #842

Merged
merged 6 commits into from Jan 25, 2017

Conversation

Projects
None yet
2 participants
@jjlk
Contributor

jjlk commented Jan 18, 2017

Hi @cdeil,
This PR (too long, sorry about that) does:

  • Replace EnergyDispersion by EnergyDispersion2D in CTAPerf
  • Add AbsorbedSpectralModel (we can kill PR #828, this one does the job)
  • Implement thresholds for TableModel and take care of NaN/Inf values that can occure because of interpolation
  • remove tests in cta_perf using point-like IRF not public (i can't put them in gammapy-extra for the moment, wait for public release). I could add some dummy IRFs at some points to have functional tests.
    Is it possible to merge? Thanks.

@jjlk jjlk changed the title from Small updates needed CTA science tools to Small updates needed for CTA science tools Jan 18, 2017

@cdeil cdeil self-assigned this Jan 18, 2017

@cdeil cdeil added this to the 0.6 milestone Jan 18, 2017

@cdeil

I left two inline comments, once those are addressed, +1 to merge.

I don't really see the advantage of the variable rename edisp -> edisp2d. There's also psf and you don't put a more specific name. But if you prefer this, OK, fine with me.

Show outdated Hide outdated gammapy/spectrum/models.py Outdated
@@ -660,10 +664,38 @@ def read_xspec_model(cls, filename, param):
return cls(energy=energy, values=values, scale_logy=False)
def evaluate(self, energy, amplitude):
interpy = self.interpy(np.log10(energy.to('eV').value))
is_array = True

This comment has been minimized.

@cdeil

cdeil Jan 18, 2017

Member

Can we try to avoid the if - else for different inputs as much as possible?

As far as I can see, all the other model evaluate functions don't do something like this.
Can't you just assume inputs are Quantity?

If this is needed for some reason, please convert to a standard (e.g. quantity or numpy array) at the top, and then the rest of the method is without if / else, just processing the data in a standard format.

@cdeil

cdeil Jan 18, 2017

Member

Can we try to avoid the if - else for different inputs as much as possible?

As far as I can see, all the other model evaluate functions don't do something like this.
Can't you just assume inputs are Quantity?

If this is needed for some reason, please convert to a standard (e.g. quantity or numpy array) at the top, and then the rest of the method is without if / else, just processing the data in a standard format.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 24, 2017

Member

@jjlk mentioned on Slack that he needs this in master for a demo tomorrow, so we agreed to leave the implementation of my comment above to a future PR.

I've fixed the docstring typo in 459eb08 . If travis-ci passes I'll merge.

Member

cdeil commented Jan 24, 2017

@jjlk mentioned on Slack that he needs this in master for a demo tomorrow, so we agreed to leave the implementation of my comment above to a future PR.

I've fixed the docstring typo in 459eb08 . If travis-ci passes I'll merge.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 25, 2017

Member

I forgot to finish this up yesterday. There was one more test fail:
https://travis-ci.org/gammapy/gammapy/jobs/194924607#L1740
which should be resolved with c8901c4 .

Merging this now.

Member

cdeil commented Jan 25, 2017

I forgot to finish this up yesterday. There was one more test fail:
https://travis-ci.org/gammapy/gammapy/jobs/194924607#L1740
which should be resolved with c8901c4 .

Merging this now.

@cdeil cdeil merged commit fb12794 into gammapy:master Jan 25, 2017

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

@cdeil cdeil changed the title from Small updates needed for CTA science tools to Add AbsorbedSpectralModel and improve CTA IRF class Jan 25, 2017

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