Follow-up from "fix relation locations"

The following discussion from !918 (merged) should be addressed:

• @sesen started a discussion:

  It's probably a bad idea to rely on the order of of relations. It might be better to specify neutral and charged relations explicitly. @jzhuo

assigned to @jzhuo

Thanks for the !918 (merged), will create the new MR to fix this tonight.

Hi @sesen

Ok, I see the !918 (merged) is actually not yet merged. Should I push things directly there or just wait until it merges and then, create a new MR to fix it?

Actually, I remember this design was trying to align the existing design in https://gitlab.cern.ch/lhcb/DaVinci/-/blob/master/Phys/DaVinci/python/DaVinci/truth_matching.py#L22

So I also need to change the DaVinci one?

You can just wait. !918 (merged) is ready to be merged.

The algorithm in truth_matching doesn't care if the relations are charged or neutral, it just combines them inside the algorithm. So if you change the order, it should be still be okay I think. You can test

In your case, giving the relations in wrong order will be an issue. So I suggest MCReconstructed: have charged_relation and neutral_relations instead of list of relations. You would also have to change the test in DaVinci using that of course.

!918 (merged) is merged now.

mentioned in merge request !924 (merged)

mentioned in merge request DaVinci!769 (merged)

added Persistency label

added DPA-WP3 label

Hello @jzhuo, is this not sorted at this point, in which case we can close?

DaVinci!769 (merged) fix this already, please close this ;)

closed