Split DREAM user guide into basic and advanced notebooks #145
Conversation
1bee196 to
0ea0ef0
Compare
nvaytet
left a comment
There was a problem hiding this comment.
Nice idea with the thumbnails. Two things:
- The basic thumbnails are displaying empty axes
- In Plopp, I saved a figures to the
_staticfolder when running the notebooks, and then used those as thumbnails instead of uploading the svgs to the git repo. This is nice as we don't need to update the images when the workflow updates, but it also means having a hidden cell at the bottom of the notebook, which is annoying if the users download the notebook.
| "outputs": [], | ||
| "source": [ | ||
| "result = workflow.compute(IofTof)\n", | ||
| "result.hist().plot()" |
There was a problem hiding this comment.
This plot is empty in the generated docs.
There was a problem hiding this comment.
Not completely empty for me but definitely broken:
This seems to be a problem with the integrated monitor norm. I don't know what. But it should not have been caused by this PR. See $146
There was a problem hiding this comment.
We should really have a test in place that catches this...
There was a problem hiding this comment.
The plot looks fine now
| "id": "31", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "We can plot the detectors individually with the help of `sc.collapse`:" |
There was a problem hiding this comment.
Instead of stacking the detectors, could we simply put them in a datagroup?
Then we don't have to remember that detector number 0 is mantle?
There was a problem hiding this comment.
We could. I thought it would be easier to process them all at once when they are stacked like this. Note that I added a 'detector' coord that contains the name.
But I can change this if you prefer a data group.
There was a problem hiding this comment.
Personally, I would go with a DataGroup.
| " da.coords['detector'].value: da\n", | ||
| " for da in sc.collapse(result, keep='tof').values()\n", | ||
| "})\n", | ||
| "split.hist().plot()" |
There was a problem hiding this comment.
Some of the results look weird, with broken lines (maybe NaNs in the data?)
There was a problem hiding this comment.
(maybe NaNs in the data?)
I think so.
But this is not a problem with the docs, AFAIK. It may be caused by the normalisation with slices the data based on the wavelength range of the monitor.
There was a problem hiding this comment.
The monitor contains NaNs. So some detector events also become NaN when normalising.
I could hide this from the basic docs by using the proton charge normalisation. Should I?
Fixed.
Could do. I wouldn't want to do it with the basic workflow though, because that is supposed to be immediately usable when downloaded. So we would have two separate solutions for the different notebooks. |
This reverts commit e4e7471.
- Clarifying comments - Show IofDspacing instead of IofTof - Define workflows in the same cells as setting params - Show graphs everywhere - Show detector names in plots
This hides the problem with monitor normalization (#161)
8f1b3a3 to
979ac00
Compare
|
@nvaytet, @celinedurniak I think I addressed all your comments. Can you take a look? I changed the section that processes all banks to use proton charge normalisation to hide the problems from #161 I don't think we need to address them in this PR. |
|
Interesting this line on the left edge of the endcap_forward (orange): It seems it is a line because other banks show a line there, but it gets boosted by the low counts in the vanadium... |
Agree. If you look at the unnormalised data, it has a line there but a much weaker one. |
This is an attempt to make the user guide more approachable. The basic notebook shows the simplest code to reduce some powder data. The advanced notebook shows how the reduction can be customised.
We may want to use different default values in the basic notebook eventually. In particular, we should figure out which normalisation will be the default.
Reviewer: please look at the rendered docs. In particular, take a look at the plots for integrated monitor norm and stacked detectors. I am not completely sure they are correct.
This supersedes #126. I think it is not feasible to have a separate notebook for each set of workflow params because that would lead to an exponential grows in notebooks. So in contrast to #126, this PR shows the individual customisation points in isolation.