Stage1 1D Deconvolution fixed for backup #859
Conversation
mvicenzi
left a comment
mvicenzi
left a comment
There was a problem hiding this comment.
I don't think it's a good strategy to introduce yet another fhicl.
I left comments below how I think these fixes can be integrated in the available flow.
|
I am trying to understand the changed logic of stage 1. To first order what Matteo has suggested is correct but I'm puzzlling why it is necessary to do that way (I think only one change necessary). But give me a minute... |
|
Oh, ok, I see. So the main change I see is that pandora used to take hits from the output of the cluster3d stage and this has been changed... but pandora can only take a single hit collection, not a vector of them so it was necessary to "combine" hits into a single east or west set of objects (which is what cluster3d output). The previous advantage to using the output of cluster 3d is that is operated as a bit of a filter so it would drop pure noise hits that did not associate across planes. When we orignially started taking data this was important, there were events that would produce so many hits that pandora would choke. It seems this is no longer happening? Anyway, there are two choices... make the changes as suggested by Matteo -or- restore pandora to operate (with 1D deconvolution) the way it did previously. Your choice either will work. |
|
Let me know when things are decided and updated and I will approve. |
|
Looks ok to me,, have you tested that it works? |
|
This seems parked... any reason to not trigger a build and see what happens? |
|
trigger build |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for e26:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
…e CRTT0Tagging for 2D was using Cluster3D instead of combineCluster3D. This was due to a typo HitLabel vs HitLabels. b) Now for 1d also the pandora hit producers are based on Cluster3D.
|
trigger build |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for e26:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
mvicenzi
left a comment
mvicenzi
left a comment
There was a problem hiding this comment.
This looks good to me, at least for the scope of this PR.
I don't fully understand the implications of cluster3D vs hits, but I agree on restoring previous behaviors for the keepup.
| @sequence::icarus_EastHits_TPC, | ||
| @sequence::icarus_WestHits_TPC, |
There was a problem hiding this comment.
Is it necessary to remove these two? I suppose it skips these producers so it makes things slighlty faster?
I don't mind removing them, as long as it doesn't break some other flow.
There was a problem hiding this comment.
I tend to agree with Matteo: unless there's some strong reason to remove them, I'd prefer to keep these elements, especially since at the moment we are in the process of revising our choices as far as signal processing is concerned and I don't want us to risk to miss something.
acampani
left a comment
acampani
left a comment
There was a problem hiding this comment.
I approve all the proposed changes, pending an explanation and possibly a change to revert the stage1_icarus_defs.fcl modifications.
This PR introduces a FCL which is able to reprocess Stage0 to produce Calibration Ntuple during Keepup.