search

lcfiplus / LCFIPlus

Flavor tagging code for ILC detectors
https://confluence.slac.stanford.edu/display/ilc/LCFIPlus
GNU General Public License v3.0
6 stars 19 forks source link

BuildUpVertexAlgorithm: Issues in moving IP tracks to the buildUp vertices: Primary vertex is not updated, beam smearing is always on #66

Open dudarboh opened 1 year ago

dudarboh commented 1 year ago

PrimaryVertexFinder has a steering parameter which determines, whether central position of the beam constrain for the fit is being smeared with a Gaussian. We don't use it by default in the ILD production steering file: <parameter name="PrimaryVertexFinder.BeamspotSmearing" type="bool" value="0" />

However, when a BuildUpVertex algorithm removes IP track (due to better chi2 with a BuildUp vertex) it refits IP vertex with a BeamspotSmearing always ON, ignoring the steering parameter!

https://github.com/lcfiplus/LCFIPlus/blob/8a4629883c1e1b8baef1feb2b6871859ecdda735/src/VertexFinderSuehara.cc#L806-L807 https://github.com/lcfiplus/LCFIPlus/blob/8a4629883c1e1b8baef1feb2b6871859ecdda735/src/VertexFinderSuehara.cc#L854-L857

Shouldn't it be consistent with a PrimaryVertexFinder?

dudarboh commented 1 year ago

More relevant issue: Refitted IP vertex with few tracks removed is not used anywhere.

There are many events, where we have identical PFOs written both in Primary and BuildUp vertices collections.

I think if we moved a track to the secondary vertex, due to the lower chi2, it should be moved, not copied.

ryonamin commented 1 year ago

@dudarboh, thank you for the heads-up! I completely agree with the first comment, I will fix this part and make a pull request as soon as possible. Regarding the second comment of duplicated tracks in Primary and BuildUp vertices, let me take a closer look which part causes the problem.

suehara commented 1 year ago

Yes tracks in both primary and secondary vertices are problematic. We need to think how to replace the primary vertex since it is not forseen on design. Also I'm a bit afraid of side effect for existing analysis but on this case fix (in default) with a fallback option to be run as before seems more reasonable for me.

dudarboh commented 1 year ago

@suehara, "I'm a bit afraid of side effect for existing analysis ..." In my opinion, it doesn't matter. If these PFO duplicates have an impact on the physics analysis and cause "better" or "worse" results/performance, this physics analysis has wrong results... I think we should not be afraid to fix this, as we should strive to get the most realistic results, not results that are backwards compatible with the old versions.

".. fallback option to be run as before .." If somebody wants to run old versions of LCFIPlus, he can easily do so for their analysis:

  1. git clone
  2. git checkout ANY commit/tag
  3. recompile & update MARLIN_DLL Maybe not everybody is aware of that, but we can write one page tutorial on how to do that, if somebody will need the old version. For the ILD Standard Reconstruction we can always postpone updating the version until we are sure it does not introduce new bugs and has only improvements.

So I am a bit skeptical about the idea of introducing new option flags to make everything backwards compatible in this particular case. In the end, its not a reconstruction algorithm option, it is a unintended bug..

". it is not forseen on design"

Yes, I agree this is a bit tricky to change. In my opinion, this associateIPTracks() should be not a part of the BuildUpAlgorithm, but rather its own, separate, algorithm. Which takes all vertices from Primary and BuildUp and then modifies both. But then we need to make verticies public so we can modify them outside of the class..

ryonamin commented 1 year ago

For the first simple part, I have made a pull request #67.

suehara commented 1 year ago

Thanks for the comment. I slightly change my mind. I think we sometime redo secondary vertexing from existing data, so maybe it's good to keep the updated primary vertex in a different collection (name). This can be done in the current framework. We should just prepare to output a new primary vertex collection from the BuildUpVertex. This does not have an impact to existing code either.