Skip to content
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 modeling notebook with model plot examples #2435

Merged
merged 7 commits into from Oct 4, 2019
Merged

Conversation

@cdeil
Copy link
Member

cdeil commented Oct 3, 2019

This PR adds a tutorials/modeling.ipynb tutorial notebook. The idea is that it will be similar to the maps intro tutorial -- showing how to work with modeling & model classes, but not execute analyses.

It's not written here - scope, content will have to be discussed a bit first and then someone has to actually write it. But I wanted to move the existing plot examples from the spatial and spectral models here, because I agree it's better to have one "model gallery" as @adonath mentioned a few days ago, and to keep the auto-generated API docs small and simple. Especially with models there is value in having different models in the same document and to be able to compare them.

@adonath - OK to go in this direction? And to limit the scope of this PR to just moving the existing examples like I did, i.e. not actually writing the notebook content now?

@cdeil cdeil added cleanup docs labels Oct 3, 2019
@cdeil cdeil added this to the 0.15 milestone Oct 3, 2019
@cdeil cdeil requested a review from adonath Oct 3, 2019
@cdeil cdeil self-assigned this Oct 3, 2019
@cdeil cdeil added this to To Do in DOCUMENTATION via automation Oct 3, 2019
@cdeil cdeil force-pushed the cdeil:models-ipynb branch from f3de9c0 to 4e37bb2 Oct 3, 2019
Copy link
Member

adonath left a comment

Thanks @cdeil! I've left one minor inline comment concerning the name of the notebook.

Some further comments:

  • In spectrum_simulation we have an example how to define a UserModel. Maybe move this to the models notebook as well?
  • Add the notebook to the tutorials/notebooks.yaml?
docs/tutorials.rst Outdated Show resolved Hide resolved
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Oct 3, 2019

In spectrum_simulation we have an example how to define a UserModel. Maybe move this to the models notebook as well?

Prefer to leave this to a later PR.

Am considering writing a PIG or wiki page to outline the docs and tutorial organisation we want for v1.0 and to have a good discussion in Oct, and then a clear plan to implement it in Nov.

Add the notebook to the tutorials/notebooks.yaml?

Was already the case

If the idea of the notebook is similar to the maps intro, then maybe rename it to intro_models.ipynb? I think "modeling" maybe suggests, that there is fitting involved...

I changed to models.ipynb, and I also changed intro_maps.ipynbs to maps.ipynb.
IMO there's no clear separation which notebooks are "intro" and which aren't, most tutorials are and will be intro. So I think not putting "intro" on some of them is more consistent.
Concerning whether it should be "modeling" or "models" I also was 50:50 when starting this PR. Which name is better will depend on how far we go in this tutorial concerning modeling framework explanation, or if we do add some fit to show how models and results come out after the fit. But OK to put "models" for now, and probably we'll limit the scope of this notebook to be the "built-in model gallery", and possibly to show how to create user-defined models.

@adonath - OK to put like this for now?
(if you want something changed, please just go ahead with a follow-up commit when merging this in)

Copy link
Member

adonath left a comment

Thanks @cdeil! No further comments from my side...

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Oct 4, 2019

@adonath ( or maybe @Bultako ?) - do you understand or can you reproduce this error?

NotImplementedError: Plotting the grid for the minor ticks is not supported.

https://travis-ci.org/gammapy/gammapy/jobs/593438153#L1344

(I can't reproduce this locally, and don't really understand it.)

For some reason, we pick up the astropy.visualization.wcsaxes axis, and there plt.grid fails?

I can see how this might happen ... in SpectralModel.plot we use the pattern we use everywhere to get the axis via plt.gca().

ax = plt.gca() if ax is None else ax

So if the sky image plotting from above in the notebook creates some non-standard axis, we will keep re-using that?
On the other hand, different cells in a Jupyter notebook seem to be decoupled from previous cells, i.e. they never overplot the content that was plotted there, no?
(e.g. the other spectral plots in that notebook and everywhere else in Gammapy work fine).

Thoughts?

@adonath - could you please also look at astropy/regions#296 and comment? Maybe we should clean up our MPL usage pattern in Gammapy also a bit, especially to avoid this accidental mixing of old wcs axis plots with later normal plots?

@cdeil cdeil assigned adonath and unassigned cdeil Oct 4, 2019
@cdeil cdeil mentioned this pull request Oct 4, 2019
@cdeil cdeil changed the title Add modeling notebook and add existing plot examples Add modeling notebook with model plot examples Oct 4, 2019
@cdeil cdeil merged commit 7d7e63e into gammapy:master Oct 4, 2019
8 of 9 checks passed
8 of 9 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
gammapy.gammapy Build #20191004.2 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
DOCUMENTATION automation moved this from To Do to Done Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
DOCUMENTATION
  
Done
2 participants
You can’t perform that action at this time.