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 model absorbed by EBL that can be fit #988

Merged
merged 9 commits into from Apr 27, 2017

Conversation

Projects
None yet
2 participants
@jjlk
Contributor

jjlk commented Apr 25, 2017

Hi @cdeil ,
As we discussed here is the PR to add a spectral model absorbed by the EBL that can be fit with sherpa. The fit is running well.

The problem arises when I want to propagate errors (for a butterfly or for flux points). I use a NDDataArray in the SpectralModelAbsorbedbyEbl.evaluate function and it can't handle the error propagation. Do you think that we can try to fix this?

I added a dedicated test to reproduce the bug. The implementation of the model could be improve and we could discuss that further when the error propagation problem will be solved.

Thanks!

@cdeil cdeil self-assigned this Apr 25, 2017

@cdeil cdeil added the feature label Apr 25, 2017

@cdeil cdeil added this to the 0.6 milestone Apr 25, 2017

@cdeil

@jjlk - I had started a code review before we skyped. Here's the inline comments.

As I said in the call, my main suggestion here would be to move the EBLAbsorption code to a separate class, and then to have the EBL absorbed spectral model take an instance of that class.

And leave the part of getting spectral_model.evaluate_error to work to a future PR. (for now, you can leave a TODO comment and a test with a @pytest.mark.xfail marker.

Show outdated Hide outdated gammapy/spectrum/models.py Outdated
Show outdated Hide outdated gammapy/spectrum/tests/test_models.py Outdated
Show outdated Hide outdated gammapy/spectrum/tests/test_models.py Outdated
Show outdated Hide outdated gammapy/spectrum/models.py Outdated

@cdeil cdeil modified the milestones: 0.7, 0.6 Apr 25, 2017

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Apr 26, 2017

Contributor

Hi @cdeil ,
As we discussed, I introduced a new class to deal with absorption (not only EBL so I removed 'EBL' everywhere), Absorption. I adapted all the ~CTA-dedicated classes and all the tests. The AbsorbedSpectralModel class have been adapted also. As you suggested, one of the test has been marked as failed (error propagation for NDDataArray).

If you are ok, I'll adapt the CTA notebook once the PR is merged. ++

Contributor

jjlk commented Apr 26, 2017

Hi @cdeil ,
As we discussed, I introduced a new class to deal with absorption (not only EBL so I removed 'EBL' everywhere), Absorption. I adapted all the ~CTA-dedicated classes and all the tests. The AbsorbedSpectralModel class have been adapted also. As you suggested, one of the test has been marked as failed (error propagation for NDDataArray).

If you are ok, I'll adapt the CTA notebook once the PR is merged. ++

@cdeil

Left some inline comments.
Main goal for now is to make all tests and the docs build pass.

Show outdated Hide outdated gammapy/spectrum/models.py Outdated
Show outdated Hide outdated gammapy/spectrum/models.py Outdated
# Target, PKS 2155-304 from 3FHL
name = 'test'
absorption = Absorption.read('$GAMMAPY_EXTRA/datasets/ebl/ebl_dominguez11.fits.gz')

This comment has been minimized.

@cdeil

cdeil Apr 26, 2017

Member

Can you make it convenient to access the "built in" EBL absorption models we provide, via

class Absorption:
    @classmethod
    def read_builtin(name):
        """"Read one of the built-in absorption models.
        Parameters
        -----------
        name : {'dominquez11', ...}
@cdeil

cdeil Apr 26, 2017

Member

Can you make it convenient to access the "built in" EBL absorption models we provide, via

class Absorption:
    @classmethod
    def read_builtin(name):
        """"Read one of the built-in absorption models.
        Parameters
        -----------
        name : {'dominquez11', ...}

This comment has been minimized.

@jjlk

jjlk Apr 27, 2017

Contributor

Can we keep that? Since people my want to use their own model.

@jjlk

jjlk Apr 27, 2017

Contributor

Can we keep that? Since people my want to use their own model.

This comment has been minimized.

@cdeil

cdeil Apr 27, 2017

Member

Yes, we should keep the read method.

I was suggesting to add another method that calls read, where the user can just select the builtin absorption model via a string, instead of having to know where the file is.

This would also serve as documentation which absorption models we have in Gammapy.
At the moment even I don't know which files are available and are supposed to work with this class.
So if you add the extra read_builtin method and then document there the list of models we have, and add a test that shows that reading and evaluating works for those, we are in better shape, no?

@cdeil

cdeil Apr 27, 2017

Member

Yes, we should keep the read method.

I was suggesting to add another method that calls read, where the user can just select the builtin absorption model via a string, instead of having to know where the file is.

This would also serve as documentation which absorption models we have in Gammapy.
At the moment even I don't know which files are available and are supposed to work with this class.
So if you add the extra read_builtin method and then document there the list of models we have, and add a test that shows that reading and evaluating works for those, we are in better shape, no?

This comment has been minimized.

@jjlk

jjlk Apr 27, 2017

Contributor

Oké done.

@jjlk

jjlk Apr 27, 2017

Contributor

Oké done.

Show outdated Hide outdated gammapy/spectrum/tests/test_models.py Outdated
Show outdated Hide outdated gammapy/spectrum/tests/test_models.py Outdated
Show outdated Hide outdated gammapy/scripts/tests/test_cta_utils.py Outdated
@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Apr 27, 2017

Contributor

HI @cdeil,
I did the the changes. Are we good?

Contributor

jjlk commented Apr 27, 2017

HI @cdeil,
I did the the changes. Are we good?

@cdeil

cdeil approved these changes Apr 27, 2017

@cdeil cdeil modified the milestones: 0.6, 0.7 Apr 27, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 27, 2017

Member

I've re-started the CI builds

Member

cdeil commented Apr 27, 2017

I've re-started the CI builds

Show outdated Hide outdated gammapy/spectrum/models.py Outdated
@requires_data('gammapy-extra')
def test_cta_simulation():
text = str(cta_simu())
assert '*** Observation summary report ***' in text

This comment has been minimized.

@cdeil

cdeil Apr 27, 2017

Member

Shouldn't this test assert on something that was computed?
(the same holds for the other tests above that only assert on str, but especially this one because it actually does some coputations, no?)

@cdeil

cdeil Apr 27, 2017

Member

Shouldn't this test assert on something that was computed?
(the same holds for the other tests above that only assert on str, but especially this one because it actually does some coputations, no?)

This comment has been minimized.

@jjlk

jjlk Apr 27, 2017

Contributor

Yes it should. But there are some randomisation and I didn't add the possibility to fix the seed. My goal was to merge these tools with Johannes class, SpectrumSimulation. Maybe for the 0.7 release? I added a small test to check if we get more than 5 sigma for detection

@jjlk

jjlk Apr 27, 2017

Contributor

Yes it should. But there are some randomisation and I didn't add the possibility to fix the seed. My goal was to merge these tools with Johannes class, SpectrumSimulation. Maybe for the 0.7 release? I added a small test to check if we get more than 5 sigma for detection

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Apr 27, 2017

Contributor

Hi @cdeil ,
Is it okay now?

Contributor

jjlk commented Apr 27, 2017

Hi @cdeil ,
Is it okay now?

Show outdated Hide outdated gammapy/spectrum/models.py Outdated
Show outdated Hide outdated gammapy/spectrum/models.py Outdated
@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Apr 27, 2017

Contributor

HI @cdeil,
Done and done. You'll love the plot ++

Contributor

jjlk commented Apr 27, 2017

HI @cdeil,
Done and done. You'll love the plot ++

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 27, 2017

Member

@jjlk - Merci pour les absorbcions de l'EBL! O la la.

Member

cdeil commented Apr 27, 2017

@jjlk - Merci pour les absorbcions de l'EBL! O la la.

@cdeil cdeil merged commit 70f7efd into gammapy:master Apr 27, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment