acts
acts copied to clipboard
feat: whiteboard alias
WIP
I noticed that a lot of information on the whiteboard rolls from one algorithm to the next but requires a different name in the process. that makes it hard to use the correct name downstream.
this PR adds an alias mechanism to the whiteboard. the alias can be changes from one entry on the whiteboard to the next. when queried the whiteboard will return the entry behind the alias. only one level of indirection is allowed. so no alias of alias.
the only way I saw to add this is through an algorithm.
also python wrappers are part of this PR.
Codecov Report
Merging #1588 (f32ed24) into main (890eff9) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## main #1588 +/- ##
=======================================
Coverage 48.54% 48.54%
=======================================
Files 384 384
Lines 21041 21041
Branches 9693 9693
=======================================
Hits 10214 10214
Misses 4105 4105
Partials 6722 6722
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
I'm wondering if it might be easier if you could set the aliases statically for all events from python. That way, you wouldn't check if the alias target exists when creating the alias, but rather when trying to resolve it.
I guess this would be tricky though because the whiteboard is created event-by-event...
I'm wondering if it might be easier if you could set the aliases statically for all events from python. That way, you wouldn't check if the alias target exists when creating the alias, but rather when trying to resolve it.
I guess this would be tricky though because the whiteboard is created event-by-event...
yeah I am also not really happy with the overhead this introduced but I don't really see an alternative. if we would do it statically we would loose the evolution of the alias e.g. particles -> particles_initial
and then particles -> particles_selected
. we would need a mechanism to remember the ordering.
Another option would be to add a convenience function to Sequencer
which internally attaches this algorithm but improves on the interface. This could even be at the python binding level I guess. I'm thinking something like
s.addAlias(particles="particles_initial")
hmm that sounds interesting. so I guess if you need the link to the algorithm/reader/writer it would look like this?
s.addAlias(algo, particles="particles_initial")
but addAlias
would need to be a C++ implementation no? or how would you imagine an implementation at the python binding level?
I think it would be good to put https://github.com/acts-project/acts/pull/1579 in first and then catch up here
:bar_chart: Physics performance monitoring for f32ed2461fede5a52b0e0308193ad6e3bac9f0d4
Full report CKF: seeded, truth smeared, truth estimated IVF: seeded, truth smeared, truth estimated Ambiguity resolution Truth tracking
Vertexing
IVF seeded
IVF truth smeared
IVF truth estimated
CKF
seeded
truth smeared
truth estimated
Ambiguity resolution
seeded
Truth tracking
Truth tracking
Looks good in general :) I just think we should address this one point before merging...
Sorry, I may have misunderstood the way this is intended to work.
I guess you want addAmbiguityResolution
to set things up so the subsequent step (addVertexFitting
) does the right thing without any special settings. Is that the goal?
If so, do we still need things like addVertexFitting(trackParameters)
settings, or are they still there just for compatibility? It might be clearer to bite the bullet and remove them from the addVertexFitting() interface.
Do I understand correctly that the aliases are only used during the Sequencer initialisation? One step could define the trackParameters
for the next step and doesn't need to know what that step is. The next step could then overwrite the trackParameters
alias for the next step's input. The important point to document is the names of the aliases we pass along, and the default values if the alias hasn't been set by a previous step.
Sorry I should have been more clear in the PR description and in the code doc.
The overall idea was to have aliases on entries of the whiteboard which are "rolling", i.e. an algorithm take in "trackParameters" and spits out "trackParameters". Downstream you would like to use the most recent "trackParameters" but since the add*
commands have no state we cannot auto discover them.
So initially I tried to put this into the whiteboard but this had a lot of overhead. @paulgessinger suggested a more static solution which made sense to me and so I ended up putting a alias state into the Sequencer.
@paulgessinger @timadye do you think documenting this is sufficient or do you see an alternative solution?
I refactored this again. the aliases are added to the sequencer which is adding it to the whiteboard. as soon as an aliased entry is added to the whiteboard it will create a second entry with the aliased name. (this also preserves your string distance mechanism @benjaminhuth ). I have to use a shared pointer instead of a unique pointer in the whiteboard but that should be fine.
let me know what you think of this version @paulgessinger @timadye @benjaminhuth