Bugfixes in SLDCorrection#143
Conversation
tmadlener
left a comment
tmadlener
left a comment
There was a problem hiding this comment.
Thanks, especially also for documenting some parts. I have a few comments / questions, see below.
| daughterHadronFlightDistance = 0.0; | ||
| daughterHadronFlightDirection = TVector3( 0.0 , 0.0 , 0.0 ); | ||
| sldVertexPosition.clear(); | ||
| if ( linkedRecoLepton->getTracks().size() == 0 ) |
There was a problem hiding this comment.
| if ( linkedRecoLepton->getTracks().size() == 0 ) | |
| if ( linkedRecoLepton->getTracks().empty() ) |
This is the recommended (and more readable) way for checking whether a vector has no elements.
There was a problem hiding this comment.
Thanks! Will include this suggestion here and everywhere where the same check occurs.
| m_pTTree1->Branch( "Lepton3DImpactParameter" , &m_Lepton3DImpactParameter ); | ||
| m_pTTree1->Branch( "OtherParticle3DImpactParameter" , &m_OtherParticle3DImpactParameter ); | ||
| h_SLDStatus = new TH1I( "SLDStatus" , ";" , 7 , 0 , 7 ); | ||
| h_SLDStatus = new TH1I( "SLDStatus" , ";" , 8 , -1 , 7 ); |
There was a problem hiding this comment.
I think the number of bins should change here as well? Or was it incorrect before? I suppose the main reason for doing this is to have access to things that haven't been classified into any of the pre-assigned categories? However, the previous version had that already if you use the under-/overflow bins.
There was a problem hiding this comment.
Before, the values were before 0/7, now -1/7 (throughout the code, floats are inserted, i.e. 0.5 -> 1. I didn't change this). According to here the third argument is the bin count, so it should be fine?
There was a problem hiding this comment.
Ah right, thanks for the clear up. I was once more confusing the order of arguments in the TH1I constructor. But as far as I can see there is no actual need for a negative bin with value -1? h_SLDStatus is only filled with the values -0.5, 0.5 and 1.5 at the moment.
| delete h_SLDStatus; | ||
| delete h_BHadronType; | ||
| delete h_CHadronType; | ||
| delete h_NuPxResidual; | ||
| delete h_NuPyResidual; | ||
| delete h_NuPzResidual; | ||
| delete h_NuEResidual; | ||
| delete h_NuPxNormalizedResidual; | ||
| delete h_NuPyNormalizedResidual; | ||
| delete h_NuPzNormalizedResidual; | ||
| delete h_NuENormalizedResidual; | ||
| delete h_recoNuPx_mcNuPx; | ||
| delete h_recoNuPy_mcNuPy; | ||
| delete h_recoNuPz_mcNuPz; | ||
| delete h_recoNuE_mcNuE; | ||
| delete h_parentPx_daughtersPx; | ||
| delete h_parentPy_daughtersPy; | ||
| delete h_parentPz_daughtersPz; | ||
| delete h_parentE_daughtersE; | ||
| delete h_recoPFOLinkedToElectron_Type; | ||
| delete h_recoPFOLinkedToMuon_Type; | ||
| delete h_SLDecayFlavour; | ||
| delete h_SLDecayModeB; | ||
| delete h_SLDecayModeC; | ||
| delete h_SLDecayOrder; | ||
| delete h_foundVertex; | ||
| delete h_secondaryVertex; | ||
| delete h_parentHadronCharge; | ||
| delete h_MCPTracks; | ||
| delete h_MCPTracks_Eweighted; | ||
| delete h_MCPTracks_Ptweighted; | ||
| delete h_FlightDirectionError; | ||
| delete h_distRecoLeptonToDownStreamVertex; |
There was a problem hiding this comment.
In older key4hep releases (e.g. 2024-04-10), there were no problems with the histograms, but in a more recent stack (e.g. 2024-10-XX), I had segfaults related to the ::end method of the SLD Correction. This affected the output of other processors as well. Maybe there was a change in how ROOT keeps track of stuff in between?
When I removed the delete statements, it was working fine.
|
Apologies, this seems to have dropped from my list somehow. Is this ready to be merged, or are is there more work to come? |
|
Is there anything missing from this or can this be merged? |
tmadlener
force-pushed
the
master
branch
5 times, most recently
from
4cef20a to
ca40934
Compare
tmadlener
force-pushed
the
master
branch
2 times, most recently
from
046ed9b to
a33848b
Compare
|
The builds that I would expect to succeed are succeeding. The diff of Can you maybe give a short summary on what actually changes on a high level? |
|
Sorry @tmadlener I did not actually mean to push to this PR. I'll clean this up and create a separate PR for the fixes to TrueJet (the idea there was to catch cases which would lead to segmentation fault, in which case TrueJet would skip the current event). |
2 participants
BEGINRELEASENOTES
ENDRELEASENOTES