-
Notifications
You must be signed in to change notification settings - Fork 11
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
Generic simple_calibration #108
base: main
Are you sure you want to change the base?
Conversation
src/simple_calibration.jl
Outdated
e_simple = e_uncal .* c | ||
e_unit = u"keV" | ||
# get peakhists and peakstats | ||
peakhists, peakstats, h_calsimple, bin_widths = get_peakhists_th228(e_simple, gamma_lines, window_sizes; binning_peak_window=binning_peak_window, e_unit=e_unit) |
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.
get_peakhists_th228
also works for generic simple_calibration_gamma
?
Or would that also need a get_peakhists_gamma
?
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.
Yes it does.
In principle we could consider re-writing/re-naming all ..._th228
calibration functions to be more generic for other gamma spectra. Some don't even need any modification at all and work out of the box for other sources.
I didn't do that to keep the changes as minimal as possible.
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.
It makes sense to rename it, but maybe this could go along then with a PR onto the dataflow where we do a search and replace for these functions.
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.
I agree. I can take care of this beginning of next year.
In that context, I'd suggest to change the julia energy-calibration metadata as well:
- Add a field "source" in the metadata, which is "th228" for LEGEND-200, but can also be something else. Like that the dataflow can pick the correct
gamma_lines
andgamma_names
from the energy calibration config.
OR
- Rename "th228_lines" etc in the metadata to "gamma_lines".
I prefer option 1, because this allows you to switch easily back and fourth between different calibration sources.
I wrote and tested this for my local Co60 data, but it should work for any other calibration spectra, because the anticipated gamma energies are passed as an argument. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 28.39% 28.82% +0.43%
==========================================
Files 37 37
Lines 3564 3594 +30
==========================================
+ Hits 1012 1036 +24
- Misses 2552 2558 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
In general, the PR looks good. Thanks @LisaSchlueter .
My comments are mainly about the naming.
Do we wanna stick to the isotope naming or start having more generic names?
@@ -490,6 +490,41 @@ end | |||
end | |||
end | |||
|
|||
@recipe function f(report::NamedTuple{(:h_calsimple, :h_uncal, :c, :peak_guess, :peakhists, :peakstats)}; cal=true) |
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.
I don't understand why this is a new recipe. Was this not here already before?
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.
This is almost a duplicate of the existing recipe. However, the old one has some stuff hardcoded, e.g. "FEP" in the plot label. I didn't want to mess with the existing one, since we're int he middle of the processing.
@@ -820,7 +855,8 @@ end | |||
if plot_ribbon | |||
ribbon := uncertainty.(report.f_fit.(0:1:1.2*value(maximum(report.x)))) | |||
end | |||
0:1:1.2*value(maximum(report.x)), value.(report.f_fit.(0:1:1.2*value(maximum(report.x)))) | |||
xstep = value(maximum(report.x))/100 |
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.
Does this also work for the 228-Th
data
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.
I tested it today on 228-Th
data and it worked well, same result.
src/simple_calibration.jl
Outdated
e_simple = e_uncal .* c | ||
e_unit = u"keV" | ||
# get peakhists and peakstats | ||
peakhists, peakstats, h_calsimple, bin_widths = get_peakhists_th228(e_simple, gamma_lines, window_sizes; binning_peak_window=binning_peak_window, e_unit=e_unit) |
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.
It makes sense to rename it, but maybe this could go along then with a PR onto the dataflow where we do a search and replace for these functions.
Thanks for your input! I really appreciate it. I suggest the following before merging this PR: LegendSpecFits:
julia-legend-dataflow:
metadata / legend-jldataflow-config:
|
That sounds like a great plan. simple_calibration_th228(args...; kwargs...) = simple_calibration_gamma(args...; source = :th228, kwargs...) where the keyword argument is |
We now have generic names
|
@theHenks Are you happy with your request? |
simple_calibration
function that can be accessed with keywordcalib_type == :gamma
which is useful for example for Co-60 data