fix SimCaloHitContribution conversion #7
Conversation
andresailer
force-pushed
the
fixCaloHitContribution
branch
from
4b94106 to
40a8ddd
Compare
Instead of adding empty contributions at least convert the numbers we know when we do not find the MCParticle Convert SimCaloHitContributions only in FillMissingCollections, because then MCParticles should have been converted
andresailer
force-pushed
the
fixCaloHitContribution
branch
from
40a8ddd to
4796684
Compare
|
I'll merge this for the nightlies, but will try to add a regression test in another PR. |
|
Thanks, yes, we need a test, |
There was a problem hiding this comment.
I essentially just have some comments on the code, we can also fix them in a later PR, as I suspect these are not the only instances of the things I commented on.
Overall, I think this looks good. In summary, what changes is that (IIUC):
- CaloHitContributions are now always populated at least with the data, even if there is no MCParticle found (as opposed to previously, where no contributions where created if no MCParticle was found)
- If an MCParticle is found than we also link them
- The whole CaloHitContribution conversion is moved to the very end, which we now have to make sure to always call
| for (int i = 0; i < edm_sch.contributions_size(); ++i) { | ||
| const auto& contrib = edm_sch.getContributions(i); |
There was a problem hiding this comment.
| for (int i = 0; i < edm_sch.contributions_size(); ++i) { | |
| const auto& contrib = edm_sch.getContributions(i); | |
| for (const auto& contrib : edm_sch.getContributions()) { |
If we are being picky here, we could remove the loop index.
| for (auto& [lcio_mcp, edm_mcp] : collection_pairs.mcparticles) { | ||
| if (edm_mcp == edm_contrib_mcp) { | ||
| mcp_found = true; | ||
| lcio_sch->addMCParticleContribution( | ||
| lcio_mcp, contrib.getEnergy(), contrib.getTime(), contrib.getPDG(), step_position.data()); | ||
| break; | ||
| } | ||
| } | ||
| } // all mcparticles | ||
| } | ||
| } | ||
|
|
||
| } // SimCaloHit | ||
| else { // edm mcp available | ||
| // std::cout << "WARNING: edm4hep contribution is not available!" << std::endl; | ||
| } |
There was a problem hiding this comment.
Can this be done with std::find? That would make this whole bit a lot easier to read, as we could remove this loop and branching structure with an intermediate boolean here.
If we want to go all in, we replace the vector<pair<LCIO*, edm4hep*>> with an actual map (or an unordered_map to avoid this linear lookup all the time. But that can probably be done at a later point.
There was a problem hiding this comment.
I can try, but changing this also needs changes in k4MarlinWrapper.
But the pointer address as key makes the ordering non-deterministic, which isn't nice and could give issues elsewhere.
|
The auto-merge didn't work because of the unresolved conversations |
Previously only contributions with all zeroes were created, when no MCParticle was found |
Instead of adding empty contributions at least convert the numbers we know when we do not find the MCParticle
Convert SimCaloHitContributions only in FillMissingCollections, because then MCParticles should have been converted
BEGINRELEASENOTES
ENDRELEASENOTES