Skip to content

Conversation

@jokasimr
Copy link
Contributor

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

@jokasimr jokasimr requested a review from nvaytet September 22, 2023 12:37
@nvaytet
Copy link
Member

nvaytet commented Sep 22, 2023

There are more than just one occurence of this in the notebook in the solution cells.
It would really be better if we could find a way to execute those cells.

I don't think there is a way to hide executed cells in a dropdown box like we did in the summer school.
It seems only Jupyterbook supports this. Even though we are currently using the jupyter book theme, and it may work now if we add the necessary tags in the cell metadata, we are moving to using the pydata sphinx theme.
It does therefore not seem worth trying to get it to work here using a theme-specific solution.

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.

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch 2 times, most recently from dfab766 to 540c686 Compare September 22, 2023 13:16
@jokasimr
Copy link
Contributor Author

jokasimr commented Sep 22, 2023

There are more than just one occurence of this in the notebook in the solution cells.

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.

@jokasimr
Copy link
Contributor Author

Do you know why the metadata field is added to the cells?

@nvaytet
Copy link
Member

nvaytet commented Sep 22, 2023

Do you know why the metadata field is added to the cells?

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.

@nvaytet
Copy link
Member

nvaytet commented Sep 22, 2023

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.

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.

@jokasimr
Copy link
Contributor Author

jokasimr commented Sep 22, 2023

possible in sphinx to change what the download button points to

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.
Something like: "To work through the tutorial yourself Download the notebook without solutions here!".

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.

Sounds good!

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch 2 times, most recently from c8fceed to 07929e0 Compare September 26, 2023 12:24
@jokasimr
Copy link
Contributor Author

jokasimr commented Sep 26, 2023

Now the flares tutorial is updated so that the solutions are kept in "code cells" instead of in "markdown code blocks".
This helps us keep the tutorial up to date by failing if the code cells fail to run.

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch from 07929e0 to 15daa3a Compare September 26, 2023 13:07
@jokasimr
Copy link
Contributor Author

jokasimr commented Sep 26, 2023

Screenshot 2023-09-26 at 15-32-53 RHESSI Solar Flares — scipp documentation

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch from 15daa3a to bf30947 Compare September 26, 2023 13:34
@jokasimr jokasimr enabled auto-merge September 26, 2023 13:34
@nvaytet
Copy link
Member

nvaytet commented Sep 26, 2023

👍
Would a log scale help on the very last plot?

@jokasimr
Copy link
Contributor Author

jokasimr commented Sep 26, 2023

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.

@nvaytet
Copy link
Member

nvaytet commented Sep 27, 2023

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.

Comment on lines -862 to -863
"to_plot = temporal_and_spatial.hist()\n",
"to_plot.transpose(['peak_time', 'y', 'x']).plot(vmin=0.0 * sc.units.one,\n",
Copy link
Member

@nvaytet nvaytet Sep 27, 2023

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'])

"id": "0f5fa729-b782-4e02-9545-bd0bf5a352b4",
"metadata": {},
"source": [
"Remove the existing binning and bin in time to yield a 1D-variable."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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)

Copy link
Contributor Author

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'?

Copy link
Member

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.

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch 2 times, most recently from 2ef7716 to 61c6cd1 Compare September 27, 2023 08:57
@nvaytet nvaytet disabled auto-merge October 2, 2023 12:07
Comment on lines 1095 to 1096
" vmin=sc.scalar(0),\n",
" vmax=temporal_and_spatial.hist().max()\n",
Copy link
Member

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",
Copy link
Member

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?

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch 2 times, most recently from 94774ce to 8ef419a Compare October 9, 2023 07:39
@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch from 8ef419a to cfefa2b Compare October 10, 2023 09:39
@jokasimr jokasimr enabled auto-merge October 12, 2023 08:29
@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch from cfefa2b to d14e1ba Compare October 12, 2023 08:29
@nvaytet
Copy link
Member

nvaytet commented Oct 16, 2023

@jokasimr the docs build is broken again, can you fix so we can finally get this one out of the way? ;-)

@jokasimr
Copy link
Contributor Author

Sorry about that, thought I had already merged this 😅

@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch 2 times, most recently from 4108bf2 to e0de9c9 Compare October 17, 2023 09:01
@jokasimr jokasimr force-pushed the fix-solar-flares-labels branch from e0de9c9 to e430aac Compare October 17, 2023 09:22
@jokasimr jokasimr merged commit d808261 into main Oct 17, 2023
@jokasimr jokasimr deleted the fix-solar-flares-labels branch October 17, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants