Conversation
verenaaur
commented
Jun 3, 2025
verenaaur
commented
- I added a 'period_channelinfo' function to the legend_data.jl file.
- This function takes a period as an argument instead of a filekey.
- It then creates a combined channelinfo of all runs in this period. It first merges all channelinfos, then creates arrays in each column for each detector (besides 'detector') with all entries from the different runs. These arrays are filtered in the next step: in columns with constant entries, only one entry remains. In columns with changing flags, e.g. "valid" or "present" in "low_aoe_status" the 'best' flag is chosen (implemented in the 'get_hierarchies' function).
- kwargs and filters ( "|> filterby...") can be used as usual.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
- Coverage 44.95% 43.64% -1.32%
==========================================
Files 27 27
Lines 2160 2225 +65
==========================================
Hits 971 971
- Misses 1189 1254 +65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
theHenks
left a comment
theHenks
left a comment
There was a problem hiding this comment.
At a first look, this looks good. However, I suggest the following:
- Let's not introduce a new function named
period_channelinfo. I highly recommend the dispatch on the existing function so that the argument is aPeriodSelLike. - All helper functions should start with a
_so that it is clear that they are not meant to be used outside of the module - Try to add types and comments to all arguments and function. Type safety and stability is important when this function will later be called very often.
- A way to cache also this
channelinfowith the help of theLRUCacheshould definitely be done - I think a lot of the code can be simplified and reduced by using more
TablesandNamedTupleoperations. I will try to help and support here once I had a look into the code in more details
| make sure to include "Tables" by "using Tables" before using this function | ||
| """ | ||
|
|
||
| function period_channelinfo(data::LegendData, period::DataPeriod; kwargs...) |
There was a problem hiding this comment.
I think this is great as a first draft, however this should become a dispatch onto the existing channelinfo
| function period_channelinfo(data::LegendData, period::DataPeriod; kwargs...) | |
| function channelinfo(data::LegendData, sel::PeriodSelLike; kwargs...) |
| """ | ||
|
|
||
| function period_channelinfo(data::LegendData, period::DataPeriod; kwargs...) | ||
| filekey_array = get_filekey_array(data, period) |
There was a problem hiding this comment.
These functions all need comments; otherwise, this is not maintainable.
| end | ||
|
|
||
| function get_filekey_array(l200, period) | ||
| rinfo = runinfo(l200, period) |> filterby(@pf $cal.is_analysis_run) |
There was a problem hiding this comment.
You are actively filtering here by cal analysis runs. This is not a good idea. Usually, the category is given to load the channelinfo, e.g.
channelinfo(l200, :p03, :r000, :cal)In the case where you load the channelinfo for a specific DataRun. This category should also be used in the case of a DataPeriod channelinfo, by allowing the DataCategory as an additional argument
|
|
||
| function get_filekey_array(l200, period) | ||
| rinfo = runinfo(l200, period) |> filterby(@pf $cal.is_analysis_run) | ||
| return [i.cal.startkey for i in rinfo] |
There was a problem hiding this comment.
This is equivalent to
| return [i.cal.startkey for i in rinfo] | |
| rinfo.cal.startkey |
| return final_chinfo | ||
| end | ||
|
|
||
| function get_filekey_array(l200, period) |
There was a problem hiding this comment.
Your function arguments need types.
| return [channelinfo(l200, fk; kwargs...) for fk in filekey_array] | ||
| end | ||
|
|
||
| function merge_and_reduce_chinfo(merged_chinfo) |
There was a problem hiding this comment.
What is this function doing? This looks like it could be simplified a lot with standard Tables operations. In particular the pushing into empty Dict or Vector is not necessary here.
| return red_chinfo | ||
| end | ||
|
|
||
| function sort_chinfo(red_chinfo) |
There was a problem hiding this comment.
What do you want to achieve with this function?
Can you not just do
sort(red_chinfoTypes are also missing.
| return sorted_chinfo | ||
| end | ||
|
|
||
| function apply_hierarchies(sorted_chinfo, hierarchies, column_order) |
| return final_chinfo | ||
| end | ||
|
|
||
| function get_hierarchies(extended::Bool) |
There was a problem hiding this comment.
I guess the base_hierarchies are static and do not need to be changed?
In that case you can just make them a const
| function get_hierarchies(extended::Bool) | |
| const _base_hierarchies = Dict{Symbol, Vector{Union{Symbol, Bool}}}( | |
| :usability => [:on, :ac, :off], | |
| :is_blinded => [true, false], | |
| :psd_usability => [:on, :off], | |
| :low_aoe_status => [:valid, :present, :missing, :unknown], | |
| :high_aoe_status => [:valid, :present, :missing, :unknown], | |
| :lq_status => [:valid, :present, :missing, :unknown], | |
| :ann_status => [:valid, :present, :missing, :unknown], | |
| :coax_rt_status => [:valid, :present, :missing, :unknown] | |
| ) |
| return base_hierarchies | ||
| end | ||
|
|
||
| function get_column_order(extended::Bool) |
There was a problem hiding this comment.
What is this function doing and can you not move this to the outer function?
There was a problem hiding this comment.
The order of the columns was changed in the prior steps and I wanted to keep the original order (as in the usual channelinfo)
2 participants
