Feature/acastill lightpropagation update - #946
Conversation
tjones79
left a comment
tjones79
left a comment
There was a problem hiding this comment.
This looks great thanks @asanchezcastillo
Few minor changes before approval :)
| } | ||
| double average_prop_time = n ? sum / n : 0; | ||
| return average_prop_time; | ||
| return n ? sum / n : 0.0; |
There was a problem hiding this comment.
This return is unreachable and looks like a copy-and-paste error. Please remove!
| } | ||
| double average_prop_time = n ? sum / n : 0; | ||
| return average_prop_time; | ||
| return n ? sum / n : 0.0; |
| else | ||
| throw std::runtime_error("LightPropagationCorrection: unexpected pdType '" + pdType + "' for opdet " + std::to_string(opdet)); |
There was a problem hiding this comment.
Should restore the throw std::runtime_error to force the code to fail loudly if it sees something unexpected!
| const std::string pdType = fPDSMap.pdType(opdet); | ||
| if(pdType=="pmt_coated" || pdType=="xarapuca_vuv") | ||
| if(fPDSMap.pdType(opdet)=="pmt_coated" || fPDSMap.pdType(opdet)=="xarapuca_vuv") | ||
| lightPropTime = std::min(lightPropTimeVIS, lightPropTimeVUV); | ||
| else if(pdType=="pmt_uncoated" || pdType=="xarapuca_vis") | ||
| else if(fPDSMap.pdType(opdet)=="pmt_uncoated" || fPDSMap.pdType(opdet)=="xarapuca_vis") |
There was a problem hiding this comment.
A minor point, I think, but it looks like the local variable assignment const std::string pdType = fPDSMap.pdType(opdet); was dropped here, resulting in calling the function multiple times in the if/else if.
If restore the local variable then that can be avoided by simply doing: if(pdType=="pmt_coated" || pdType=="xarapuca_vuv") as in the original code
| //OpFlash to OpHit | ||
| art::FindManyP<recob::OpHit> flashToOpHitAssns_tpc0(opflashListHandle_tpc0, e, fOpFlashLabel_tpc0); | ||
| art::FindManyP<recob::OpHit> flashToOpHitAssns_tpc1(opflashListHandle_tpc1, e, fOpFlashLabel_tpc1); | ||
| flashToOpHitAssns_tpc0 = std::make_unique<art::FindManyP<recob::OpHit>>( opflashListHandle_tpc0, e, fOpFlashLabel_tpc0); |
There was a problem hiding this comment.
Is there a reason these have to be declared as such?
I naively think these should be kept consistent with the above associations in how they are declared.
Could you move these back to local variables? You can pass them by const & into your new CorrectOpFlash() method. (Note: This is already done for slice_opt0finder_assns, slice_hit_assns etc... just above this line).
You would need to remove the corresponding declerations in the header file here
| // Flash ophit associations | ||
| std::unique_ptr<art::FindManyP<recob::OpHit>> flashToOpHitAssns_tpc0; | ||
| std::unique_ptr<art::FindManyP<recob::OpHit>> flashToOpHitAssns_tpc1; |
There was a problem hiding this comment.
| fLightPropCorrectionLabel = p.get<std::string>("LightPropCorrectionLabel"); | ||
| } | ||
|
|
||
| void LightPropagationCorrectionAna::analyze(art::Event const& e) |
There was a problem hiding this comment.
Could you also add fParticlePropagationTime = -99999.; and fPhotonPropagationTime = -99999.; to this block just to keep the event-by-event initialisation consistent?
2 participants
Description
This PR does:
Checklist
Reviewers,AssigneesDevelopementRelevant PR links (optional)
Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)?
This PRs should be merged with
SBNSoftware/sbnobj#175
SBNSoftware/sbncode#666
SBNSoftware/sbnanaobj#195
Link(s) to docdb describing changes (optional)
Is there a docdb describing the issue this solves or the feature added?