Helper and notebook for removing vanadium peaks #66
Conversation
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "d = pipeline.compute(FocussedDataDspacing[VanadiumRun])\n", |
There was a problem hiding this comment.
This could probably do with some explanation as to what it is plotting?
| "version": "3.10.13" | ||
| } | ||
| }, | ||
| "nbformat": 4, |
There was a problem hiding this comment.
I'm not sure I see peak fitting or removal used in the notebook? Am I missing something?
There was a problem hiding this comment.
I committed the wrong notebook 🤦. Please have another look! (It looks like sphinx raised a warning about the missing notebook but that didn't fail the build.)
jl-wynen
force-pushed
the
strip-vana-peaks
branch
from
1394656 to
87b2333
Compare
| "Even though the peaks are small for vanadium, we need to remove them to extract pure incoherent scattering.\n", | ||
| "We can approximate the coherent scattering contribution by fitting functions to the peaks and subtracting those fitted functions.\n", | ||
| "[scippneutron.peaks](https://scipp.github.io/scippneutron/generated/modules/scippneutron.peaks.html) contains general functionality for fitting and removing peaks.\n", | ||
| "Here, we use it through [ess.powder.external.powgen.peaks](../../generated/modules/ess.powder.external.powgen.peaks.rst) which provides useful defaults for vanadium peaks at POWGEN.\n", |
There was a problem hiding this comment.
Once #68 will be merged, the path will be ess.snspowder.powgen. The files should also be moved around. Hopefully this won't be too annoying.
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "incoherent = scn.peaks.remove_peaks(sc.values(to_fit), fit_results)\n", |
There was a problem hiding this comment.
[Unrelated to this PR] Are the fit_vanadium_peaks and scn.peaks.remove_peaks designed to be used in pairs, on the same data, or could you imagine have some fit_results from another run and you use them to remove_peaks on a new Vanadium data?
I guess the height of the peaks etc would become an issue?
There was a problem hiding this comment.
I don't think it makes sense to use fit results from another run. SO they would always be used in pairs. We could have a combined function. But the split is useful for inspecting fits and also fudging them. E.g., Celine and I had a case where a fit was marked as failed because of a bad p-value. But it was good enough to use for removing a peak. To do so, we had to go in and modify the FitResult to mark it as successful.
There was a problem hiding this comment.
Yes and it is also useful for making the diagnostics plot.
Should we add those functions into a workflow graph so we have a workflow which computes a "vanadium without peaks" or something like that?
There was a problem hiding this comment.
I prefer to not automate this step. The automated diagnostics are not good enough to check whether the result is ok. Users should inspect the fit results manually. This is why I created a new notebook.
There was a problem hiding this comment.
In the reflectometry workflow, johannes added some types which were plots instead of data. You could then pipeline.compute(DiagnosticPlot) to view the figure.
Maybe something like this coudl also work here?
That said, I'm not so bothered, leaving it as is also works.
There was a problem hiding this comment.
This could be done for diagnostics. But we may still need to modify the fit results before removing peaks. I'd say we leave it as is until we have more insight. In particular from DREAM.
jl-wynen
force-pushed
the
strip-vana-peaks
branch
from
7c1f65a to
720aa7e
Compare
| " if xlim is not None:\n", | ||
| " def in_range(x: sc.Variable) -> bool:\n", | ||
| " return sc.isfinite(x) and (xlim[0] <= x) and (x < xlim[1])\n", | ||
| " data = data[data.dim, xlim[0]:xlim[1]]\n", | ||
| " removed = removed[removed.dim, xlim[0]:xlim[1]]\n", | ||
| " fit_results, peak_estimates = zip(*(\n", | ||
| " (r, e)\n", | ||
| " for r, e in zip(fit_results, peak_estimates, strict=True)\n", | ||
| " if in_range(r.window[0]) and in_range(r.window[1])\n", | ||
| " ), strict=True)\n", |
There was a problem hiding this comment.
I changed this because the behaviour suddenly changed on my machine. I don't know what caused it.
Now, when using ax.set_xlim the text shown with ax.text is still shown. This messes up how the axes are shown in a figure. The new solution removes everything that is not within the shown range. It may remove a fit that lies at the edge of the range, though.
Fixes #24.
I only added it in the POWGEN module because the default fit parameters depend on the data. And we don't have DREAM data that could be used here. But it should be straight forward to transfer it to DREAM once we have that data.