fix: Fix timed clusterization with duplicate cells#4762
fix: Fix timed clusterization with duplicate cells#4762DJDuque wants to merge 3 commits intoacts-project:mainfrom
Conversation
|
| // Cells with same position but time difference larger than tolerance should | ||
| // not be considered duplicates | ||
| CellCollection cells = {Cell(0ul, 0, 0, 0.0), Cell(1ul, 0, 0, 1.0)}; |
There was a problem hiding this comment.
I was under the impression that the readout will always only provide one value per cell and will have to choose which one to pass on.
If such a readout exists we should ultimately support it, but I worry a bit about implications for reconstruction without time information.
There was a problem hiding this comment.
Hi @andiwand , sorry I didn't follow up on this sooner. I got a bit too busy.
Could you expand a bit on what you mean by "...but I worry a bit about implications for reconstruction without time information."?
As it is, I think this current PR doesn't change at all the behavior of clustering without time information.
Basically, if I am understanding correctly:
- A readout that only provides one value per cell will work exactly the same before and after this change (both for timed and non-timed clustering).
- A readout that provides more than one value per cell (with different times), will work exactly the same as before if passed through clustering without time information. Additionally, it will now work correctly with timed information.
Also, I am not sure if such a readout exists, so maybe it just makes sense to not add this until someone with such needs complains about it. Nonetheless:
- I think it'll be a tricky issue to identify for someone with such a readout. The current implementation will just silently give the "wrong" results. Same issue as this.
- As far as I can see, it doesn't impact in any way current usages of clusterization.
There was a problem hiding this comment.
1. I think it'll be a tricky issue to identify for someone with such a readout. The current implementation will just silently give the "wrong" results. Same issue as [this](https://github.com/acts-project/acts/issues/4736#issuecomment-3459115392).
Actually, the above might be incorrect. Maybe the current implementation after this fix already throws. I'll have to double check.
Hi @AJPfleger , from my side, I think this PR is done and ready to be reviewed/merged. I think that @andiwand marked it as a draft because he mentioned having some concerns on "... implications for reconstruction without time information". Maybe @andiwand can comment a bit more on this. I think that this, in its current state, should behave exactly as it does for "reconstruction without time information", while also being useful in the case of cells in the same geometric position and different times. |
acts-project-service
added
the
Fails Athena tests
andiwand
removed
the
Fails Athena tests
There was a problem hiding this comment.
I am no expert on clustering and its ACTS implementation, so what I say might very well be wrong, but what still seems odd to me is that there is TimedClusterization and Clusterization and we have to touch Clusterization to make time work. I worry a bit that this could have at least some performance impacts on pure spatial clustering.
That the CCL generalizes to time seems plausible to me
Tagging people who worked on the clustering recently: @CarloVarni @goetzgaycken
| if (spaceCompatibility == Acts::Ccl::ConnectResult::eDuplicate || | ||
| spaceCompatibility == Acts::Ccl::ConnectResult::eConn) { | ||
| return (timeDiff < timeTolerance) ? spaceCompatibility | ||
| : Acts::Ccl::ConnectResult::eNoConn; | ||
| } |
There was a problem hiding this comment.
| if (spaceCompatibility == Acts::Ccl::ConnectResult::eDuplicate || | |
| spaceCompatibility == Acts::Ccl::ConnectResult::eConn) { | |
| return (timeDiff < timeTolerance) ? spaceCompatibility | |
| : Acts::Ccl::ConnectResult::eNoConn; | |
| } | |
| if ((spaceCompatibility == Acts::Ccl::ConnectResult::eDuplicate || | |
| spaceCompatibility == Acts::Ccl::ConnectResult::eConn) && | |
| (timeDiff > timeTolerance)) { | |
| return Acts::Ccl::ConnectResult::eNoConn; | |
| } |
seems a bit conciser to me but no strong opinion
4 participants
In timed clusterization, if multiple cells are at the same row and column, they are no longer considered duplicates unless the times of both are also within the
timeTolerance.Additionally, having multiple cells in the same position with large time differences was also preventing actual connections to be found. This is fixed by modifying the
Connectionsbuffer size to the actual maximum number of possible connections including time differences.--- END COMMIT MESSAGE ---