Reboost core API #26
Reboost core API #26
Conversation
|
CI will be fixed once legend-exp/legend-pydataobj#132 is merged. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
===========================================
+ Coverage 54.87% 66.76% +11.88%
===========================================
Files 16 21 +5
Lines 1139 1390 +251
===========================================
+ Hits 625 928 +303
+ Misses 514 462 -52 ☔ View full report in Codecov by Sentry. |
|
we will also need this in the reboost CI workflow: https://github.com/legend-exp/legend-pygeom-hpges/blob/bd48f219d964dd6dd31697b00b8a97297ded6e5b/.github/workflows/main.yml#L34-L37 |
|
for the optics, focusing on the example config would not work yet, thugh? There we had the insertion of output tables into inputs. This is apparently not possible with your code right now. Apart from that, I cannot really remember the reasoning for that split in the first place.... (maybe this was never intended to be run sequentially, but were two options "working on energy depositions alone" and "getting pe numbers for each channel" and that interdependence slipped in....?) we just would have to figure out how to do this - this would maybe require on-demand building of a GLM for the output tables, if they are used as an input in a later step. But we still could figure that out later? |
What do you mean by this? What I wrote gives a mapping of input --> output detectors. So you can build all sipms tables starting from the scintillator input_table (and only reading it once). Just a matter of the right config and proccesors
I didn't depend any dependence, I intended them to be totally independent. Maybe my filler name for the "scintillator" input was confusing? |
|
see this excerpt from the example config. I do not really remember if this dependency there was intentional in the end. (I remeber that we discussed such thing in December, but I do not remember the outcome) But anyway, this does not make sense in the config. I guess it was a mistake from writing the example config file back then. |
|
Yes sorry this is my mistake the sipms input should be Lar or whatever
…________________________________
From: Manuel Huber ***@***.***>
Sent: Thursday, January 30, 2025 8:01 PM
To: legend-exp/reboost ***@***.***>
Cc: Dixon, Toby ***@***.***>; Author ***@***.***>
Subject: Re: [legend-exp/reboost] Reboost core API (PR #26)
⚠ Caution: External sender
see this excerpt from the example config. I do not really remember if this dependency there was intentional in the end. (I remeber that we discussed such thing in December, but I do not remember the outcome)
But anyway, this does not make sense in the config.
- name: lar
detector_mapping:
- output: scintillators
[...]
- name: spms
detector_mapping:
- output: [...]
input: scintillators
—
Reply to this email directly, view it on GitHub<#26 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANSET47GONUHFGB6DTZ5UYD2NJZH3AVCNFSM6AAAAABV7KSRX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRVGMZTGNBRHA>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
There was a problem hiding this comment.
I don't have major comments, so this can be merged from my side. It's also a lot of code and I'll need more time to read it.
About performance: there are surely many functions that can be sped up with Numba, but we need to have a detailed profiling first to identify the bottlenecks.
|
Also please rebase on |
|
hmm, now you did something very weird to your git history. This branch is now not rebased on main at all, and is completely unmergeable, as it has conflicts in a large number of files... I am really not sure how you managed to do this, but I suppose you will have to fix it so it can be merged... |
|
Yes I messed up something, I will fix it
…________________________________
From: Manuel Huber ***@***.***>
Sent: Saturday, February 1, 2025 1:51 PM
To: legend-exp/reboost ***@***.***>
Cc: Dixon, Toby ***@***.***>; Author ***@***.***>
Subject: Re: [legend-exp/reboost] Reboost core API (PR #26)
⚠ Caution: External sender
hmm, now you did something very weird to your git history. This branch is now not rebased on main at all, and is completely unmergeable, as it has conflicts in a large number of files...
I am really not sure how you managed to do this, but I suppose you will have to fix it so it can be merged...
—
Reply to this email directly, view it on GitHub<#26 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANSET47APCQCQ27Y5R2BIJL2NS7MLAVCNFSM6AAAAABV7KSRX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRYHE2DCNJXGM>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Ok now we are back to how it was before, I will try again to rebase |
…dd an example proccesor (activness)
Co-authored-by: Manuel Huber <[email protected]>
|
Looks good. From my side itwould be okay to merge, but please still disuss Luigi's commets with him. |
|
@gipert I added a |
Implemented most of the core functions (orchestration) and added tests. @gipert , @ManuelHu
Still missing is extracting the detector mapping from config (for cases its not trivial) and also more full scale tests with
build_hit