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

Improve docs for SpectrumFit #1345

Merged
merged 1 commit into from Apr 4, 2018

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Mar 20, 2018

  • clarifiy meaning of spectrum_fit.model (now it the untouched input model)
  • make flux point estimation a separate step in SpectrumAnalysisIACT

UPDATE: This PR was triggered by #1333 . After discussing offline with @cdeil we agreed that improving the docs is the best way to go.

@joleroi joleroi added the cleanup label Mar 20, 2018

@joleroi joleroi added this to the 0.8 milestone Mar 20, 2018

@joleroi joleroi self-assigned this Mar 20, 2018

@joleroi joleroi requested a review from cdeil Mar 20, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 20, 2018

@joleroi - is it a good idea to add the second point you mention above to this PR? (up to you)

For what is here: I'm confused by all those models. What is background_model? Please add docstrings for the properties. And usually one goes via the properties mostly even from within the class, while you currently go via the _ hidden data members.

Let me know when this PR is ready for review, and please put a better PR title (for the changelog we just copy & paste PR titles).

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 22, 2018

@joleroi - is it a good idea to add the second point you mention above to this PR? (up to you)

I don't know, it was your idea.

About the models: I thinks it's pretty clear the way it was before, but that might be related to the fact that I wrote the code. I don't want to spend much time on this. So either someone comes up with concrete suggestions or I suggest we close this and #1333

@joleroi joleroi changed the title from Adress #1333 to Improve docs for SpectrumFit Mar 26, 2018

@joleroi joleroi added the docs label Mar 26, 2018

@cdeil

cdeil approved these changes Mar 26, 2018

@joleroi - These changes look good!

Above you have a task list to also improve SpectrumAnalysisIACT here. OK, if you want, but you can also merge this one and make a new PR for that other change.

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 4, 2018

CI fails are unrelated.

@joleroi - I'd suggest you merge this in now, and leave the second task to a separate PR.

@joleroi joleroi merged commit dc7a7f6 into gammapy:master Apr 4, 2018

1 of 2 checks passed

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

@joleroi joleroi deleted the joleroi:improvements branch Apr 4, 2018

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