acts icon indicating copy to clipboard operation
acts copied to clipboard

feat: whiteboard alias

Open andiwand opened this issue 2 years ago • 6 comments

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.

andiwand avatar Oct 07 '22 16:10 andiwand

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

codecov[bot] avatar Oct 07 '22 17:10 codecov[bot]

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...

paulgessinger avatar Oct 08 '22 06:10 paulgessinger

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.

andiwand avatar Oct 08 '22 08:10 andiwand

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")

paulgessinger avatar Oct 08 '22 09:10 paulgessinger

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?

andiwand avatar Oct 08 '22 10:10 andiwand

I think it would be good to put https://github.com/acts-project/acts/pull/1579 in first and then catch up here

andiwand avatar Oct 19 '22 14:10 andiwand

: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

github-actions[bot] avatar Oct 26 '22 09:10 github-actions[bot]

Looks good in general :) I just think we should address this one point before merging...

benjaminhuth avatar Oct 27 '22 14:10 benjaminhuth

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.

timadye avatar Oct 27 '22 17:10 timadye

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?

andiwand avatar Oct 28 '22 08:10 andiwand

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

andiwand avatar Nov 01 '22 18:11 andiwand