-
Notifications
You must be signed in to change notification settings - Fork 10
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 21.33% 21.24% -0.09%
==========================================
Files 36 36
Lines 3305 3365 +60
==========================================
+ Hits 705 715 +10
- Misses 2600 2650 +50 ☔ View full report in Codecov by Sentry. |
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
# remove calib type from kwargs | ||
@assert haskey(kwargs, :calib_type) "Calibration type not specified" | ||
calib_type = kwargs[:calib_type] | ||
# remove :calib_type from kwargs | ||
kwargs = pairs(NamedTuple(filter(k -> !(:calib_type in k), kwargs))) | ||
if calib_type == :th228 | ||
@debug "Use simple calibration for Th228 lines" | ||
return simple_calibration_th228(e_uncal, th228_lines, window_sizes,; kwargs...) | ||
return simple_calibration_th228(e_uncal, gamma_lines, window_sizes,; kwargs...) | ||
elseif calib_type == :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.
Why not call this :co60
if it is for the 60-Co
data? My original idea was here to have something for different isotopes since you always look for different features in the data.
But I am actually open for other suggestions.
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 works also for any calibration data. I think we should have only 1 generic simple_calibration
function in the future and only special one if we can't make it work with the generic one
src/simple_calibration.jl
Outdated
|
||
|
||
function simple_calibration_th228(e_uncal::Vector{<:Real}, th228_lines::Vector{<:Unitful.Energy{<:Real}}, window_sizes::Vector{<:Tuple{Unitful.Energy{<:Real}, Unitful.Energy{<:Real}}},; n_bins::Int=15000, quantile_perc::Float64=NaN, binning_peak_window::Unitful.Energy{<:Real}=10.0u"keV") | ||
function simple_calibration_th228(e_uncal::Vector{<:Real}, th228_lines::Vector{<:Unitful.Energy{<:Real}}, window_sizes::Vector{<:Tuple{Unitful.Energy{<:Real}, Unitful.Energy{<:Real}}},; n_bins::Int=15000, quantile_perc::Float64=NaN, binning_peak_window::Unitful.Energy{<:Real}=10.0u"keV", e_unit::Unitful.EnergyUnits=u"keV") |
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 always had the unit and therefore the scale fixed in the code since for some of the values I assumed that everything from there on is in keV
. Is there a specific reason why to have this as a kwarg
? In my opinion, it should be fine to define the "energy scale" in the code itself.
src/simple_calibration.jl
Outdated
* `binning_peak_window`: window size for the peak search histogram (default: 10 keV) | ||
* `peak_quantile`: quantile range for the peak search (default: 0.5..1.0) | ||
""" | ||
function simple_calibration_gamma(e_uncal::Vector{<:Real}, gamma_lines::Vector{<:Unitful.Energy{<:Real}}, window_sizes::Vector{<:Tuple{Unitful.Energy{<:Real}, Unitful.Energy{<:Real}}},; n_bins::Int=15000, quantile_perc::Float64=NaN, binning_peak_window::Unitful.Energy{<:Real}=10.0u"keV", peak_quantile::ClosedInterval{<:Real} = 0.5..1.0, e_unit::Unitful.EnergyUnits=u"keV") |
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 also rename to simpel_calibration_gamma
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 |
simple_calibration
function that can be accessed with keywordcalib_type == :gamma
which is useful for example for Co-60 data