v4 to v5 Port #48
Merged
v4 to v5 Port #48
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removed Fragment Separation From TAData & TPData Classes
Remove C Python Binding Import
aeoranday
added
the
maintenance
|
Hi Alex, Thanks for the work on this. Here is my first round of review: |
Also included a link to the channel maps table.
Added and using an option to use integer x-ticks in PDFPlotter histograms.
Previously invisible.
Log minor ticks were not being colored correctly.
Users should define which maker/plugin in a config JSON.
The last TP is sometimes missing, so skip it. Removed a pandas deprecation warning.
This is more correct to PEP 484.
Indexing the reader class now returns the indexed content of the primary data.
Follows the flow change that happened in `trigger`.
ArturSztuc
approved these changes
Oct 7, 2024
Comment on lines
+248
to
+259
| char* payload = static_cast<char*>(malloc(payload_size)); | ||
|
|
||
|
|
||
| size_t offset(0); | ||
| for ( const auto& ta : ta_buffer ) { | ||
| triggeralgs::write_overlay(ta, payload+offset); | ||
| triggeralgs::write_overlay(ta, static_cast<void*>(payload+offset)); | ||
| offset += triggeralgs::get_overlay_nbytes(ta); | ||
| } | ||
|
|
||
| daqdataformats::Fragment ta_frag(payload, payload_size); | ||
| daqdataformats::Fragment ta_frag(static_cast<void*>(payload), payload_size); | ||
|
|
||
| free(payload); | ||
| free(static_cast<void*>(payload)); |
There was a problem hiding this comment.
Both here and in EmulationUnit, I would totally avoid using malloc and free. This is low-level C that should only be used if really cannot be avoided. In this case we could simply do:
std::vector<char> payload(payload_size);
size_t offset(0);
for ( const auto& ta : ta_buffer ) {
triggeralgs::write_overlay(ta, static_cast<void*>(payload.data()+offset));
offset += triggeralgs::get_overlay_nbytes(ta);
}
daqdataformats::Fragment ta_frag(static_cast<void*>(payload.data()), payload_size);
And no free would be needed, since vector would go out of scope anyway (but could clear it if you must).
BUT this is not related to v4-to-v5 movement, so I'm fine if that's done at later date with a separate PR
| @@ -80,18 +80,19 @@ EmulationUnit<T, U, V>::emulate_vector(const std::vector<T>& inputs) { | |||
| if (payload_size == 0) | |||
| return nullptr; | |||
|
|
|||
| void* payload = malloc(payload_size); | |||
| // Awkward type conversion to avoid compiler complaints on void* arithmetic. | |||
| char* payload = static_cast<char*>(malloc(payload_size)); | |||
There was a problem hiding this comment.
ditto re. malloc & free, could use vector here instead.
| @@ -212,6 +212,7 @@ int main(int argc, char const *argv[]) | |||
| if (latencies) { | |||
| std::filesystem::path output_path(output_file_path); | |||
| ta_emulator->set_timing_file((output_path.parent_path() / ("ta_timings_" + output_path.stem().string() + ".csv")).string()); | |||
| ta_emulator->write_csv_header("TP Time Start,TP ADC Integral,Time Diffs,Is Last TP In TA"); | |||
There was a problem hiding this comment.
I like this, although I wish it was done in a separate PR, not in a simple "v4 to v5". I'm now having to do PR review commit-by-commit :(
There was a problem hiding this comment.
I understand the out-of-scopeness, but it was part of the requested changes. I tried to keep the commits well separated for what they changed.
| @@ -7,6 +7,7 @@ | |||
| import trgdataformats | |||
|
|
|||
| import numpy as np | |||
| from numpy.typing import NDArray | |||
There was a problem hiding this comment.
Had no idea numpy has its own typing module. Nice.
Only removes from building. The cxx is kept for reference.
MRiganSUSX
reviewed
Oct 14, 2024
There was a problem hiding this comment.
Thanks @aeoranday for improvements, it's already better.
Here are more comments after another round of review:
- running
process_tpstreamover the same tpstream hd5 file, same configuration (barring change in algo names, but both prescale):
V4:
Average TA Time Process (19702 TAs): 7410 ns.
Average TC Time Process (197 TCs): 7758 ns.
-- Finished TSL 118490:0
V5:
Average TA Time Process (19695 TAs): 18111 ns.
Average TC Time Process (197 TCs): 21811 ns.
-- Finished TSL 118490:0
=> different number of TAsm and much much slower?
(output file also very slightly smaller - guessing that's the 'missing' TAs ?)
- running
process_tpstreamwith--latencies:
-- (same issue as above with missing TAs)
-- the 'Time Diffs' parameters differ for the same TA(s):
V4:
108017674586074187,16506,7949,1 <- ??
108017674565021675,26052,147,0
V5:
108017674586074187,16506,8115,0
108017674565021675,26052,953,0
-
convert_tplatencies:
what exactly is the point of convert_tplatencies? It does not seem that there any scripts available that would use the output of this. And, if what it produces is the desired result, why don't we get it straight from tp_process with --latencies? is that inbetween output used for anything else other than the input to this converter ? -
tp_dump:
still get weird TPs (attach ss)
-
tc_dump:
- when using on the output file made with
process_tpstream(the input to that process step is the same in both versions):
even though the # of TCs made IS the same, the TCs are different, as the TC stats are different:
V4:
time_candidate
Reference Statistics:
Total # TCs = 10,
Mean = 2.13,
Std = 1.37,
Min = 0.0,
Max = 4.220520496368408.
Anomalies:
# of >3 Sigma TCs = 0,
# of >2 Sigma TCs = 0.
time_end
Reference Statistics:
Total # TCs = 10,
Mean = 2.13,
Std = 1.37,
Min = 0.0,
Max = 4.220515489578247.
Anomalies:
# of >3 Sigma TCs = 0,
# of >2 Sigma TCs = 0.
time_start
Reference Statistics:
Total # TCs = 10,
Mean = 2.13,
Std = 1.37,
Min = 0.0,
Max = 4.220520496368408.
Anomalies:
# of >3 Sigma TCs = 0,
# of >2 Sigma TCs = 0.
V5:
time_candidate
Reference Statistics:
Total # TCs = 10,
Mean = 2.02,
Std = 1.46,
Min = 0.0,
Max = 4.214995384216309.
Anomalies:
# of >3 Sigma TCs = 0,
# of >2 Sigma TCs = 0.
time_end
Reference Statistics:
Total # TCs = 10,
Mean = 2.02,
Std = 1.46,
Min = 0.0,
Max = 4.214994430541992.
Anomalies:
# of >3 Sigma TCs = 0,
# of >2 Sigma TCs = 0.
time_start
Reference Statistics:
Total # TCs = 10,
Mean = 2.02,
Std = 1.46,
Min = 0.0,
Max = 4.214995384216309.
Anomalies:
# of >3 Sigma TCs = 0,
# of >2 Sigma TCs = 0.
(using this dump on the same raw hdf5 (without the process_ step), the output is identical).
py-trgtools:
-- very last command block (Plotting->Example) is missing command to load data:
tpr.read_all_fragments() (or something similar that takes subset)
without that:
>>> print(tpr['adc_peak'])
[]
-- the output example.pdf seems to be broken / corrupted (empty) file (tried 3 different inputs)
-- there is an error message when exiting the interactive python session:
Exception ignored in: <function PDFPlotter.__del__ at 0x7fb183882680>
Traceback (most recent call last):
File "/nfs/home/mrigan/dune-daq/v5.2-241010/install/trgtools/lib64/python/trgtools/plot/PDFPlotter.py", line 91, in __del__
File "/nfs/home/mrigan/dune-daq/v5.2-241010/.venv/lib/python3.10/site-packages/matplotlib/backends/backend_pdf.py", line 2721, in close
File "/nfs/home/mrigan/dune-daq/v5.2-241010/.venv/lib/python3.10/site-packages/matplotlib/backends/backend_pdf.py", line 828, in finalize
File "/nfs/home/mrigan/dune-daq/v5.2-241010/.venv/lib/python3.10/site-packages/matplotlib/backends/backend_pdf.py", line 974, in writeFonts
File "/nfs/home/mrigan/dune-daq/v5.2-241010/.venv/lib/python3.10/site-packages/matplotlib/backends/backend_pdf.py", line 1451, in embedTTF
File "/nfs/home/mrigan/dune-daq/v5.2-241010/.venv/lib/python3.10/site-packages/matplotlib/backends/backend_pdf.py", line 1182, in embedTTFType3
ImportError: sys.meta_path is None, Python is likely shutting down
I understand that lot (if not all) of these are not caused by your port. I'm just listing my findings here as I was going through testing.
Please, for those points that you are not fixing for this PR, make issues.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This simply ports this repository from v4 to v5.
Most of the dependency for this port was data format related, which have already been ported.
Testing was done by viewing
t*_dump.pyfor various files from both v4 and v5 and confirming the results are sensible.This also features a slight change to docstrings and a missed enhancement in v4.
The usage of
process_tpstreamis consistent and was previously addressed when the naming change happened intriggeralgsfor v5.