Update to Vertex Reconstruction

[flemmons]

Added FineGrid search and mean time selection

[marc1uk]

hi Franklin, Apologies for the delay, i finally got the CI up and running so i'm now clearing out the PRs as you can see.

Firstly, can i trouble you to re-introduce the (int) casts in loops that you removed? (you can see them in the 'Files Changed', or with git diff). Those are there to suppress compiler warnings about comparison of signed and unsigned integer. In fact, for the new loops you've added in, please also either add similar (int) casting, or (better yet), use an unsigned loop iterator: for(size_t i=0; i< thing->size(); ++i) for example.

Secondly and more importantly, it looks like you're leaking memory in VertexSeedGenerator::FindCenter - you define a few things on the heap with new, but never delete them. Similarly in MeanTimeCheck you declare 5 histograms and a TFile on the heap in Initialise but don't delete them.

You've also introduced a whole lot of whitespace changes in FoMCalculator so that essentially the whole file appears changed, which makes it very difficult to see what actually changed (both in reviewing PRs, but also when comparing across commits); would you be able to just apply the code changes, without changing the whitespace?

Thanks

[flemmons]

Hi Marcus, I've addressed most of your edits in the last comment: I've re-added the (int) modifier where I removed it (convention has its place, and I can understand not wanting unnecessary warnings). I've also removed VtxSeedGenerator's GenerateFineGrid function altogether to avoid the memory issues. It was redundant with the VtxSeedFineGrid tool, which accomplished the same task more fully. As far as FoMCalculator goes, it wasn't adding whitespace, but removing some strange ^G or something from all lines a while ago. I think it came in from using a few different softwares and operating systems on my end. Somewhere along the line, it wound up with an unnecessary endline character. Regardless, the main change there was the addition of an experimental penalty term added to the calculation. I hadn't meant to include this in this update, so I've commented it out (for now; I intend to come back to this, and will remove it completely at that point. Let me know if there are any further concerns. Cheers

[marc1uk]

Hi Franklin, Thanks for the changes, and sorry this is taking me a while to clear out these PRs. It looks like you still have the declaration of GenerateFineGrid, FindCenter and FindSimpleDirection in VertexSeedGenerator.h, but those functions are no longer defined. Would you be able to remove the associated declarations if those are no longer needed?

You also still have memory leaks in VtxSeedFineGrid. myFoMCalculator is leaked in FindCenter. I don't really see why this needs to be allocated on the heap; simply declaring it on the stack would fix the memory leak, and should in general be the default unless you specifically need to pass something between scopes. Likewise, although it isn't leaked vtxSeedFineGrid::FindSimpleDirection.cpp:L364 could declare a RecoVertex on the stack with RecoVtx newVertex;. The caller then wouldn't have to worry about deleting it.

Finally, the new vector vSeedVtxList constructed in Initialise is immediately leaked on the first Execute at line 38. I see you have an associated delete in Finalise, but it's commented out. Generally that's a sign that someone realises they probably should be deleting something, but doing so causes crashes. :sweat_smile: Please do bring it up at a software meeting, or in the slack, and we can try to find a solution that neither leaks nor crashes. In this specific case I think it should be sufficient simply to instead set vSeedVtxList=nullptr in Initialise, instead of calling new, and I would indeed leave the delete call on line 96 commented out.

In fact, although it's not your code, I'd be surprised if the equivalent delete call in VtxSeedGenerator::Finalise at line 94 did not result in a double free when the ToolChain terminates. When pointers are passed to a BoostStore::Set call the BoostStore takes ownership of the pointed-to object (something i personally disagree with, but that's how it is for now). Deleting that object in Finalise would then mean the BoostStore would be trying to delete an already-deleted object, which I would expect to cause a (admittedly harmlessly) crash during termination. If you're running a ToolChain with the VtxSeedGenerator Tool in and do happen to see this behaviour, try removing that delete call on Line 94 and see if it fixes it.

If there's anything unclear here, please do raise it at the software meeting tomorrow.

[flemmons]

Marcus, New update. I've fixed the errors you've outlined here. I have, at least for now, left the things that are relatively harmless be, but the memory leaks should be 'plugged.' Let me know if there are any further issues. There's also some new experimental charge fit in progress, but it shouldn't cause any problems, since it don't use many pointers, and are locked behind new config variables. There shouldn't be any leaks, since there aren't any new pointers. It's really only included because this pull request is attached to my working repo, but it's not necessarily important to the original point of this pr.

[flemmons]

Correction: now it should cause no problems. Some syntactical errors had slipped through the previous update.

[marc1uk]

hi franklin. I really have to apologise for taking so long to get this merged. I can't see any showstoppers so I'll resolve the conflicts with Unity and Factory and merge it in provided the CI says it's ok.

There are a couple of very minor points that it would be good to make in a future PR.

There are a few things declared new in LikelihoodFitterCheck::Initialise, but no delete calls. It's possible the Histograms may be owned by the TFile created above (which is also not deleted), but TGraphs do not by default become owned by the currently open file and would need deleting manually. Some of these declared items don't even seem to be used, so it's possible they could also just be removed.

There are some trailing cout calls that it would be good to either remove or convert to Log.