-
Notifications
You must be signed in to change notification settings - Fork 21
docs: fix solar flares tutorial plot labels #3258
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
Conversation
|
There are more than just one occurence of this in the notebook in the solution cells. I don't think there is a way to hide executed cells in a dropdown box like we did in the summer school. Maybe we should just not hide the solutions, but give them directly in the notebook, like we have for the other tutorials. It would also be nice to have the plots on the page, as they are quite nice. |
dfab766 to
540c686
Compare
Yes I noticed that, I'll update it. Yeah definitely agree it would be nice to execute the cells, and display the results in the documentation page. How about having two versions of the notebook? One with solutions and one without? The one without solutions could be auto-generated. Then we can display the one with solutions in the documentation page, and link to a download button for the one without solutions so people can try it out themselves. |
|
Do you know why the |
I think it's just newer versions of Juputer that do that, compared to the one that was used to first make this notebook a year or so ago. Note that it didn't create the metadata, but simply added things inside it. |
This is basically what we have done for the summer school. I think generally this is the way to go, but we would have to do this for the other tutorial we have to be consistent. Plus, I do not know if it is possible in sphinx to change what the download button points to. All that to say that I don't think it's worth the effort at this point. But we should probably do something like that if we start running more Scipp courses. |
I was thinking more along the lines of adding an anchor tag near the top of the page that points to a file download from our github.
Sounds good! |
c8fceed to
07929e0
Compare
|
Now the flares tutorial is updated so that the solutions are kept in "code cells" instead of in "markdown code blocks". |
07929e0 to
15daa3a
Compare
15daa3a to
bf30947
Compare
|
👍 |
|
Tried with a log scale but felt it didn't make it more readable. It might be good to show how to create a log scale though, so maybe good to include anyway. |
Not needed, we have this documented elsewhere. You can leave it as is. |
| "to_plot = temporal_and_spatial.hist()\n", | ||
| "to_plot.transpose(['peak_time', 'y', 'x']).plot(vmin=0.0 * sc.units.one,\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could keep this in, by using the slicer plot to show time slices of xy maps?
E.g. pp.slicer(temporal_and_spatial.hist(), keep=['y', 'x'])
docs/tutorials/solar_flares.ipynb
Outdated
| "id": "0f5fa729-b782-4e02-9545-bd0bf5a352b4", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Remove the existing binning and bin in time to yield a 1D-variable." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Remove the existing binning and bin in time to yield a 1D-variable." | |
| "Remove the existing binning and bin in time to yield a 1D-variable representing the number of flares in each time bin." |
which could maybe remove the need for having p.canvas.ylabel = 'number of flares' multiple times below?
(I think the unit would then just be counts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the change that removed the need for having ylabel = 'number of flares'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That we say 'number of flares' in the text, so that we don't have to write it on the figures and use the default 'counts' instead.
2ef7716 to
61c6cd1
Compare
docs/tutorials/solar_flares.ipynb
Outdated
| " vmin=sc.scalar(0),\n", | ||
| " vmax=temporal_and_spatial.hist().max()\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use autoscale='fixed' to get the same result I think? See https://scipp.github.io/plopp/reference/generated/plopp.slicer.html
| " keep=['y', 'x'],\n", | ||
| " vmin=sc.scalar(0),\n", | ||
| " vmax=temporal_and_spatial.hist().max()\n", | ||
| ")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add aspect='equal', cmap='magma', norm='log' to stay consistent with the other figures?
94774ce to
8ef419a
Compare
8ef419a to
cfefa2b
Compare
cfefa2b to
d14e1ba
Compare
|
@jokasimr the docs build is broken again, can you fix so we can finally get this one out of the way? ;-) |
|
Sorry about that, thought I had already merged this 😅 |
4108bf2 to
e0de9c9
Compare
e0de9c9 to
e430aac
Compare

Fix plotting example in documentation. The
labelsargument is not allowed and raises an error.