PMT-only Vertex Reconstruction #361
Conversation
jminock
left a comment
jminock
left a comment
There was a problem hiding this comment.
Did not spot any memory leaks or logical issues. Everything is well-documented (thank you!) and should not impede any other Tools or ToolChains
jminock
added
Ready for Level 0 Review and merge
and removed
Ready for Software meeting discussion
labels
removed event skipping - PMTWaveformSim will be adjusted
marc1uk
left a comment
marc1uk
left a comment
There was a problem hiding this comment.
Code in general looks good, just a few places where we can reduce a lot of duplication with some templating or lambdas.
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| std::vector<Position> VertexLeastSquares::GenerateVetices() // this is Andrew's fault there's a typo (and no I'm not going to fix it :) ) |
| std::vector<Position> VertexLeastSquares::GenerateVetices() // this is Andrew's fault there's a typo (and no I'm not going to fix it :) ) | ||
| { | ||
|
|
||
| if (fExternalSeeding) { |
There was a problem hiding this comment.
There's an if/else branch here of ~50 odd lines on each branch, but as far as I can see the only difference between them is whether to add the buffers to yMin, yMax, and max_radius. It seems you could just do
if (fExternalSeeding) {
yMin -= fYBuffer;
yMax x= fYBuffer;
max_radius += fRadialBuffer;
}
and totally eliminate any code duplication.
|
|
||
|
|
||
| //------------------------------------------------------------------------------ | ||
| void VertexLeastSquares::EvalAtGuessVertexMC(util::Matrix &A, util::Vector &b, |
There was a problem hiding this comment.
can you guess what I'm gonna say...? 🙃
This is wholly identical to EvalAtGuessVertex, other than our constant headache of taking vector<Hit> instead of vector<MCHit>. There are two simple ways to deal with this here:
- make
EvalAtGaussVertexa templated method. Only downside of this is the implementation needs to move to the header file. - use a lambda. Downside is that lambdas can't be members so wrapping is a bit awkward. One way could be as follows:
void VertexLeastSquares::EvalAtGuessVertex(const std::vector<Hit>& hits, const std::vector<MCHit>& mchits, ... ){
auto process = [](auto& hits){
// all current code in EvalAtGaussVertexMC can go here unchanged
}
if(hits) process(hits);
else process(mchits);
}
This keeps the EvalAtGuessVertex code within a member function without moving the implementation to the header, while taking advantage of lambdas' ability to accept auto to work with both types.
I'm happy to accept either (or something else), but please do one or the other.
| // We need at least 4 hits | ||
| if (filt_hits.size() < 4) { | ||
| std::cout << "Not enough hits. We only have: " << filt_hits.size() << std::endl; | ||
| fVertexMap->emplace(clusterpair.first, Position(-99, -99, -99)); |
There was a problem hiding this comment.
is a vertex necessary? can the map simply not have an entry?
with c++17 we could use std::optional.... but for now, perhaps as a minimum using an outside the tank position would be helpful.... 😕
| } | ||
| } | ||
|
|
||
| // Don't let the loop last forever |
There was a problem hiding this comment.
while(true){
++loop_num;
if(loop_num > max_loop_num) break;
}
.... so a for loop then? 😅
| double max_vertical = fGeom->GetTankHalfheight() + fExternalVertexMaxHeight; | ||
|
|
||
| if (radial_dist > max_radial || std::abs(dy) > max_vertical) { | ||
| continue; // reject this seed |
There was a problem hiding this comment.
should this comment be 'reject this guess'
| } | ||
|
|
||
| //------------------------------------------------------------------------------ | ||
| void VertexLeastSquares::RunLoopMC() |
There was a problem hiding this comment.
another duplicate function
|
|
||
|
|
||
| //------------------------------------------------------------------------------ | ||
| std::vector<MCHit> VertexLeastSquares::FilterHitsMC(std::vector<MCHit> hits) |
There was a problem hiding this comment.
i'm assuming this can also be merged with FilterHits
3 participants
Describe your changes
VertexLeastSquaresis a PMT-based vertex reconstruction tool that can be used for low energy events (NC-like, neutrons, etc...) without an MRD track to assist in the reconstruction. More info can be found on the included README, and in Andrew's slides.It can be used on MC hits, MC waveform-reconstructed hits, and data hits. It is designed to run over clusters to yield a reconstructed vertex (x,y,z) and stdev to the timing residuals.
Checklist before submitting your PR
newusage, there is a reason the data must be on the heapnewthere is adelete, unless I explicitly know why (e.g. ROOT or a BoostStore takes ownership)^ store should take care of the
newused in the tool (line 134, 135) but it would be nice to double check.