merge or unify DAClusterizerInZT_vect and DAClusterizerInZ_vect

[slava77]

This is a followup to #19935

DAClusterizerInZT_vect was derived primarily from DAClusterizerInZ_vect, but with some deviations that were either made ad hoc or to preserve historical compatibility with the old settings in DAClusterizerInZT.

The class declaration and implementation of DAClusterizerInZ_vect and DAClusterizerInZT_vect are over 1K lines of code and differ perhaps only by 10% or less.

Divergent places were noted in comments to #19935

@lgray

[cmsbuild]

A new Issue was created by @slava77 Slava Krutelyov.

@davidlange6, @Dr15Jones, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[slava77]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

[slava77]

@lgray

please clarify on the status of this issue now that we have the 4D development in shape

[lgray]

With the newer implementation, templating in dimensionality is likely even easier. I'll give it a try soon.

…

On Nov 16, 2017 09:41, "Slava Krutelyov" ***@***.***> wrote: @lgray <https://github.com/lgray> please clarify on the status of this issue now that we have the 4D development in shape — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBMOYRy4i18jK3J7DliPji1sq98fsT8ks5s3Em3gaJpZM4PP28z> .

[perrotta]

@werdmann @lgray: with the code cleaning implemented in #32764 and the followup bug fix applied with #32924 some work was restarted on DAClusterizerInZT_vect and DAClusterizerInZ_vect, in view of a planned further development for the primary vertex producer targeted at CMSSW_11_3
Before starting with the actual development, could you please comment about the possible fulfilling of this long lasting and never resolved issue? Is there any plan to unify the code, as it was assumed to be "even easier" more than three years ago (and I think the latest cleanings is heading even more in that direction), or is it something that it is not taken into account any more?

[werdmann]

@perotta Although I try to keep the codes similar where possible to improve the maintainability, I am not considering unifying the two, at least in the sense that we have one code that does either Z or ZT. It is not fully clear to me at this point into which direction the ZT clustering will develop in the long run. They may in some sense diverge, e.g. when particle mass hypotheses are introduced for timing, or they may become more monolithic in the sense that they use the same clustering up to a point and only diverge for the last iterations because timing only becomes relevant at the lowest temperatures.

@werdmann @lgray: with the code cleaning implemented in #32764 and the followup bug fix applied with #32924 some work was restarted on DAClusterizerInZT_vect and DAClusterizerInZ_vect, in view of a planned further development for the primary vertex producer targeted at CMSSW_11_3
Before starting with the actual development, could you please comment about the possible fulfilling of this long lasting and never resolved issue? Is there any plan to unify the code, as it was assumed to be "even easier" more than three years ago (and I think the latest cleanings is heading even more in that direction), or is it something that it is not taken into account any more?

[perrotta]

Thank you @werdmann

Then I think that this issue could be closed as "won't fix": do you agree @slava77?

[slava77]

Thank you @werdmann

Then I think that this issue could be closed as "won't fix": do you agree @slava77?

minimizing code replication is still important and we have sort of a triplicate in DA now (non-vect, Z, and ZT). I'd rather keep pushing (if it can be called pushing at once in a couple of years rate).

[smuzaffar]

@cms-sw/reconstruction-l2 any update on this?