Bug fix where no vertex exists #146
Conversation
There was a problem hiding this comment.
Many thanks @absolution1 - main comment about the LArMvaHelper::MvaFeature.
| } | ||
| catch (const StatusCodeException &) {} | ||
| } | ||
|
|
||
| if (error) | ||
| else if (!pVertexList->empty()) |
There was a problem hiding this comment.
So e.g. if (!vertexDistance.IsInitialized() && .. && etc...)
| { | ||
| CaloHitList threeDCaloHitList; | ||
| LArPfoHelper::GetCaloHits(pInputPfo, TPC_3D, threeDCaloHitList); | ||
| vertexDistance = (pVertexList->front()->GetPosition() - (threeDCaloHitList.front())->GetPositionVector()).GetMagnitude(); // if n3dHits == 1, can't calculate vertex postion of input pfos hence ask for the position of the single 3D hit instead and set vertexDistance to be the magnitude of the difference between the interaction vertex and the hit's position. |
There was a problem hiding this comment.
Suggest lose this comment, and add a commit to correct any whitespace/indentation in the file, thank you.
| } | ||
|
|
||
| featureVector.push_back(vertexDistance); | ||
| featureVector.push_back(std::numeric_limits<LArMvaHelper::MvaFeature>::lowest()); |
There was a problem hiding this comment.
So an uninitialised vertexDistance should be what is wanted here, but obviously tests required.
| @@ -460,6 +460,7 @@ void ThreeDVertexDistanceFeatureTool::Run(LArMvaHelper::MvaFeatureVector &featur | |||
| if (PandoraContentApi::GetSettings(*pAlgorithm)->ShouldDisplayAlgorithmInfo()) | |||
| std::cout << "----> Running Algorithm Tool: " << this->GetInstanceName() << ", " << this->GetType() << std::endl; | |||
|
|
|||
| LArMvaHelper::MvaFeature vertexDistance; | |||
| const VertexList *pVertexList(nullptr); | |||
| (void) PandoraContentApi::GetCurrentList(*pAlgorithm, pVertexList); | |||
|
|
|||
There was a problem hiding this comment.
What do you think about i) checking whether pVertexList is not nullptr here, and ii) checking to see whether the list isn't empty, up front, here? Because can't calculate vertexDistance below if there's no vertex. Could then remove some vertex checks below, but that might actually make implementation less robust against future changes.
There was a problem hiding this comment.
Good shout, doing the sanity checks earlier is more in keeping with the other pandora code snippets I've seen.
I've added this in.
One point, I've slightly broken the style of the function ie. the vertex list sanity check fills the feature vector and returns. However, the more complicated checks still only set vertexDistance and then wait for the final push_back at the end of the function.
There was a problem hiding this comment.
In the update above to get the MvaFeature type consistency for the vertex distance, the value used has been reset to zero in the case where the vertex list is empty, which, from discussion elsewhere, I think is not what is wanted.
There was a problem hiding this comment.
H @AndyChappell
My intention was to return a default-initialised vertexDistance when the actual vertex distance couldn't be calculated, which I think is what the original code tried to do. Is that not the case?
There was a problem hiding this comment.
Ah I saw the email notification for your other comment (but cannot find it here bizarrely).
The very original plan was to return an extreme value for a default but that was before I knew that the type was wrapped in an initialisation bool. So, I opted to preserve how the original code handled defaults.
There was a problem hiding this comment.
This looks good to me now (one small cosmetic comment). We should't merge this until we've discussed with @AndyChappell (who's looking after the big picture right now), but if you want this for rapid testing, can push to a nicely-named feature branch?
| const VertexList *pVertexList(nullptr); | ||
| (void) PandoraContentApi::GetCurrentList(*pAlgorithm, pVertexList); | ||
|
|
||
| if (!pVertexList || pVertexList->empty()){ |
There was a problem hiding this comment.
Typical style is to have the brace { on the next line 😄
absolution1
force-pushed
the
mvafeaturebugfix
branch
from
2feb215
to
81e7dbf
Compare
|
I've rebased my commits so the comments are probably going to be a bit mangled... |
absolution1
changed the base branch from
master
to
feature/larpandoracontent_v03_21_01
Figured it might be easier to discuss this in a PR than on slack