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 default parameters for spectral models #1090

Merged
merged 5 commits into from Jul 27, 2017

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Jul 25, 2017

This PR addresses #992 and introduces default values for the spectral model parameters.

Once question I have is, whether we prefer strings as defaults (as suggested by @cdeil in #992).

A little bit tricky was the choice for parameter min / max default values. I've tried make the choice as general as possible and removed some of the previous default settings (OK with you @joleroi?).

The PR also adds example plots for the spectral models.

@adonath adonath added the feature label Jul 25, 2017

@adonath adonath added this to the 0.7 milestone Jul 25, 2017

@adonath adonath requested review from joleroi and cdeil Jul 26, 2017

@cdeil

cdeil approved these changes Jul 26, 2017

I had a quick look at the diff. Don't have much time this week to look in detail. Would suggest you just go ahead and merge.

The only comment I have is whether it makes sense to add a plot example for each class to the API docs, because our docs build is already so slow, and I don't think it's very useful to have those plots, is it?

A different option that I think would be simpler to maintain and useful for users would be to show off examples of models and plotting here:
https://nbviewer.jupyter.org/github/gammapy/gammapy-extra/blob/master/notebooks/spectrum_models.ipynb
A method that returns a Jupyter widget where people can play with parameter values and see how the spectrum changes would be best and probably even possible to implement as a generic method in the base class with 10 lines of code, no?
If you think it's a good idea, you could do it here or make a reminder issue.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 26, 2017

Member

For my last suggestion, we would also need a list of available spectral models, which definitely should be implemented as a registry. That would also make usage much simpler where people don't have to import each spectral model, but can use that registry and a factory method SpectralModel.make('powerlaw', pars=...). So overall it's not a small and quick addition as I first thought and probably just a separate PR, and the only thing to consider here is whether it really makes sense to add a plot directive to each spectral model class docstring.

Member

cdeil commented Jul 26, 2017

For my last suggestion, we would also need a list of available spectral models, which definitely should be implemented as a registry. That would also make usage much simpler where people don't have to import each spectral model, but can use that registry and a factory method SpectralModel.make('powerlaw', pars=...). So overall it's not a small and quick addition as I first thought and probably just a separate PR, and the only thing to consider here is whether it really makes sense to add a plot directive to each spectral model class docstring.

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jul 27, 2017

Member

@cdeil I removed the plot directive, but kept the code as an example. I'm fine to just showcase the models in a dedicated notebook. I'll merge now.

Member

adonath commented Jul 27, 2017

@cdeil I removed the plot directive, but kept the code as an example. I'm fine to just showcase the models in a dedicated notebook. I'll merge now.

@adonath adonath merged commit 78ffcdd into gammapy:master Jul 27, 2017

2 checks passed

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

@cdeil cdeil changed the title from Introduce spectral models default parameters to Add default parameters for spectral models Aug 31, 2017

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