Persist Slice BDT Inputs #189
Conversation
| m_featureMap["NuNFinalStatePfos"] = nuNFinalStatePfos; | ||
| m_featureMap["NuNHitsTotal"] = nuNHitsTotal; | ||
| m_featureMap["NuVertexY"] = nuVertexY; | ||
| m_featureMap["NuWeightedDirZ"] = nuWeightedDirZ; | ||
| m_featureMap["NuNSpacePointsInSphere"] = nuNSpacePointsInSphere; | ||
| m_featureMap["NuEigenRatioInSphere"] = nuEigenRatioInSphere; | ||
| m_featureMap["CRLongestTrackDirY"] = crLongestTrackDirY; | ||
| m_featureMap["CRLongestTrackDeflection"] = crLongestTrackDeflection; | ||
| m_featureMap["CRFracHitsInLongestTrack"] = crFracHitsInLongestTrack; | ||
| m_featureMap["CRNHitsMax"] = nCRHitsMax; | ||
|
|
There was a problem hiding this comment.
This is not necessarily a comment for now but it would be 'nice' for this to be automated in such a way that we don't have to manually append lines here if we come up with new features.
There was a problem hiding this comment.
I agree, I discussed this offline with @AndyChappell but without significant changes to the MvaFeatureVector there didn't seem to be an obvious clean way of achieving this.
There was a problem hiding this comment.
I guess one way would be not creating the interim variables (like nuFinalStatePfos) and instead storing the calculations straight onto the map. There would need to be some cleanup functionality to nuke the map if an exception gets thrown though...
There was a problem hiding this comment.
Yes, in principle we could populate the map as each variable is defined/updated and then populate the feature vector from the map. You do then lose some of the intuitiveness of the variable types from declaring everything as a double, but maybe reduces the chance that you miss a variable in any update?
There was a problem hiding this comment.
I'm happy to implement this change if you think this would be better. I think in some ways it will becomes less obvious to follow the logic but as you say it may protect from future mistakes in adding variables.
There was a problem hiding this comment.
I have mixed views about it, would be curious to hear @johnmarshall80 's thoughts?
There was a problem hiding this comment.
Hmmm, can't think of anything great immediately. Could (although it could be easily subverted) just pass the addition to the vector and map over to a member function? E.g. This would take the string and the double, and just do the vector push back and map insertion, via operator[]?
At the moment it doesn't look like any of this has any explicit error handling, but you could of course have the calls to this member function (to add to map and vector) within a try/catch block, with both map and vector wiped clean if there's a relevant exception raised for any one feature?
(Note that BeamParticleId tool currently very similar implementation, but presumably up to e..g ProtoDUNE if they'd like these features persisted too, then would need to follow approach chosen here.)
There was a problem hiding this comment.
I think this is probably something for a tidying up PR. I noticed that there's a hardcoded magic number a little about here (line 395 in Henry's PR). So, there could for sure be a bit of sprucing up. Addressing this could be done as part of that sprucing.
|
Looks nice to me @henrylay97 |
| @@ -255,13 +256,33 @@ void NeutrinoIdTool<T>::SelectPfosByProbability(const pandora::Algorithm *const | |||
| { | |||
| object_creation::ParticleFlowObject::Metadata metadata; | |||
| metadata.m_propertiesToAdd["NuScore"] = nuProbability; | |||
|
|
|||
| if (m_persistFeatures) | |||
There was a problem hiding this comment.
This will need a run through clang format.
| @@ -147,6 +154,7 @@ class NeutrinoIdTool : public SliceIdBaseTool | |||
|
|
|||
| bool m_isAvailable; ///< Is the feature vector available | |||
| LArMvaHelper::MvaFeatureVector m_featureVector; ///< The MVA feature vector | |||
| std::map<std::string, double> m_featureMap; ///< A map between MVA features and their names | |||
There was a problem hiding this comment.
Could add a typedef for this, as per LArMvaHelper::MvaFeatureVector above, and use throughout?
| @@ -259,6 +267,9 @@ class NeutrinoIdTool : public SliceIdBaseTool | |||
| float m_minProbability; ///< Minimum probability required to classify a slice as the neutrino | |||
| unsigned int m_maxNeutrinos; ///< The maximum number of neutrinos to select in any one event | |||
|
|
|||
| // Persistence | |||
There was a problem hiding this comment.
Unnecessary, I'd say, already covered by comment attached to member variable below.
|
Hey all - wanted to check in on the status of this PR? Sorry for bothering but have been following a bit and have not seen comments in a bit so wanted to check :) thanks in advance for info! |
|
Hi Bruce, I'll look to merge this one in either at the end of this week or the beginning of next, so hopefully it will be available via next week's LArSoft release. |
|
Thanks for the update! |
henrylay97
force-pushed
the
feature/hlay_persist_slice_bdt_inputs
branch
from
4b38ac3
to
6a2a994
Compare
AndyChappell
changed the base branch from
master
to
feature/larpandoracontent_v03_26_04
Add the functionality to persist the features used in the slice ID MVA but only with an opt-in xml parameter, should have no change to current functionality.