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 Parameter class to SpectralModel #871

Merged
merged 6 commits into from Feb 10, 2017

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Feb 6, 2017

Work in progress

I did not use the Parameter class in gammapy.utils.modelling. There, the entire class hirarchy in gammapy.spectrum.models is duplicated - so clearly someone needs to take the time to merge these two efforts. During this step also the Parameter classes can easily be merge.

@joleroi joleroi added the feature label Feb 6, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 7, 2017

Member

@joleroi - I suggest we spend the time this week and merge the parameter, parameter list and model classes. I think it's mostly straight-forward and just needs to be done? Maybe you finish up your PRs and when we try to do some pair-coding in the coming days? (E.g. I'm completely free Thursday afternoon.)

Member

cdeil commented Feb 7, 2017

@joleroi - I suggest we spend the time this week and merge the parameter, parameter list and model classes. I think it's mostly straight-forward and just needs to be done? Maybe you finish up your PRs and when we try to do some pair-coding in the coming days? (E.g. I'm completely free Thursday afternoon.)

@cdeil cdeil added this to the 0.6 milestone Feb 7, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 7, 2017

Member

I did not use the Parameter class in gammapy.utils.modelling. There, the entire class hirarchy in gammapy.spectrum.models is duplicated - so clearly someone needs to take the time to merge these two efforts. During this step also the Parameter classes can easily be merge.

OK to merge if you want. But that merging will have to be done in the next few days anyways.
A compromise could be that you merge the Parameter and ParameterList classes you're adding here into the existing classes in gammapy.utils.modeling instead of introducing new ones, and leave the merging of the spectral model classes for later.

Member

cdeil commented Feb 7, 2017

I did not use the Parameter class in gammapy.utils.modelling. There, the entire class hirarchy in gammapy.spectrum.models is duplicated - so clearly someone needs to take the time to merge these two efforts. During this step also the Parameter classes can easily be merge.

OK to merge if you want. But that merging will have to be done in the next few days anyways.
A compromise could be that you merge the Parameter and ParameterList classes you're adding here into the existing classes in gammapy.utils.modeling instead of introducing new ones, and leave the merging of the spectral model classes for later.

joleroi added some commits Feb 8, 2017

@joleroi joleroi changed the title from WIP: Add Parameter class to SpectralModel to Add Parameter class to SpectralModel Feb 9, 2017

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 9, 2017

Contributor

@adonath and I merged the Parameter class into gammapy.utils.modelling

Contributor

joleroi commented Feb 9, 2017

@adonath and I merged the Parameter class into gammapy.utils.modelling

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 9, 2017

Member

🎉

(my preference would be that you just merge and I try it out in the coming days, unless you have a specific question or want me to review it)

Member

cdeil commented Feb 9, 2017

🎉

(my preference would be that you just merge and I try it out in the coming days, unless you have a specific question or want me to review it)

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 10, 2017

Contributor

There's one remaining test failure I don't understand

https://travis-ci.org/gammapy/gammapy/jobs/200271615#L1174

@adonath this is your code isn't it? Can you have a look. The issue is related to using the spectral models with the uncertainties package. It works in model_with_uncertaintes because of this hack. In the failing test the uncertainties package is used in a different way (that I don't understand).

Contributor

joleroi commented Feb 10, 2017

There's one remaining test failure I don't understand

https://travis-ci.org/gammapy/gammapy/jobs/200271615#L1174

@adonath this is your code isn't it? Can you have a look. The issue is related to using the spectral models with the uncertainties package. It works in model_with_uncertaintes because of this hack. In the failing test the uncertainties package is used in a different way (that I don't understand).

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 10, 2017

Contributor

As discussed offline I xfailed the failing test

Contributor

joleroi commented Feb 10, 2017

As discussed offline I xfailed the failing test

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 10, 2017

Contributor

failing tests related to gammacat will be fixed in TODO (@cdeil)

Contributor

joleroi commented Feb 10, 2017

failing tests related to gammacat will be fixed in TODO (@cdeil)

@joleroi joleroi merged commit 0149fca into gammapy:master Feb 10, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@joleroi joleroi deleted the joleroi:add_parameter_class branch Feb 10, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 10, 2017

Member

I'm working on the gammacat tests in #815 and will try to fix things there.
(or xfail there and leave it to next week)

Member

cdeil commented Feb 10, 2017

I'm working on the gammacat tests in #815 and will try to fix things there.
(or xfail there and leave it to next week)

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 10, 2017

Member

@joleroi - What about this fail (I see it locally, didn't check on travis-ci yet).
Can you see it? Fix it?

______________________________________________________________ TestSpectrumFitResult.test_model_with_uncertainties _______________________________________________________________

self = <gammapy.spectrum.tests.test_results.TestSpectrumFitResult object at 0x1604609e8>

    @requires_dependency('uncertainties')
    def test_model_with_uncertainties(self):
>       actual = self.fit_result.model_with_uncertainties.parameters.index.s
E       AttributeError: 'ParameterList' object has no attribute 'index'
Member

cdeil commented Feb 10, 2017

@joleroi - What about this fail (I see it locally, didn't check on travis-ci yet).
Can you see it? Fix it?

______________________________________________________________ TestSpectrumFitResult.test_model_with_uncertainties _______________________________________________________________

self = <gammapy.spectrum.tests.test_results.TestSpectrumFitResult object at 0x1604609e8>

    @requires_dependency('uncertainties')
    def test_model_with_uncertainties(self):
>       actual = self.fit_result.model_with_uncertainties.parameters.index.s
E       AttributeError: 'ParameterList' object has no attribute 'index'
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 10, 2017

Contributor

Fixed in 7127967

Contributor

joleroi commented Feb 10, 2017

Fixed in 7127967

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