Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse the direction of the ParticleID relation(s) #268

Merged
merged 22 commits into from
May 1, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Feb 8, 2024

BEGINRELEASENOTES

  • Remove the relations from Cluster to ParticleID
  • Remove the relations from ReconstructedParticle to ParticleID
  • Add a OneToOneRelation from ParticleID to a ReconstructedParticle
  • This is a breaking change for both existing files and interfaces. There will be no schema evolution for this change
  • Add a new edm4hep::utils::PIDHandler and some related utility functionality to help with handling the necessary meta data.

ENDRELEASENOTES

Fixes #251, Fixes #217

Associated PRs (need to be merged at the same time to not break the nightlies completely)

Open questions (so far):

  • Does the ParticleID still need the type and algorithmType members. In principle at least the latter now can be entirely encoded in the ParticleID collection, since in principle each algorithm can (should?) produce its own collection?
    • Probably not strictly, but we keep it around for now to not complicate this further.

@tmadlener
Copy link
Contributor Author

I have added a first version of a PIDHandler class that can reverse the direction of the ParticleID -> ReconstructedParticle relation.

Since EDM4HEP::utils now becomes an actual shared library, I have renamed the target because otherwise we will just get a libutils.so which might clash. To ease the transition for now, I add back the EDM4HEP::utils target for downstream consumers, but we could also consider simply breaking things entirely.

@tmadlener tmadlener force-pushed the particleid-reversal branch 2 times, most recently from 949fa99 to ddb4d7f Compare April 19, 2024 11:46
Copy link
Collaborator

@andresailer andresailer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation! It only allows me to offer a minor nitpick.

doc/PIDHandler.md Outdated Show resolved Hide resolved
doc/PIDHandler.md Outdated Show resolved Hide resolved
doc/PIDHandler.md Outdated Show resolved Hide resolved
doc/PIDHandler.md Outdated Show resolved Hide resolved
doc/PIDHandler.md Outdated Show resolved Hide resolved
doc/PIDHandler.md Outdated Show resolved Hide resolved
Comment on lines 80 to 82
for (const auto& pid : getPIDs(reco)) {
if (pid.getAlgorithmType() == algoType) {
return pid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const auto& pid : getPIDs(reco)) {
if (pid.getAlgorithmType() == algoType) {
return pid;
const auto& [begin, end] = m_recoPidMap.equal_range(reco);
for (auto it = begin; it != end; ++it) {
if (it->getAlgorithmType() == algoType) {
return *it;

The vector of PIDs doesn't need to be created

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going for some code reuse, but good point. Changed.

Comment on lines +82 to +105
**Finally most of the examples assume that the desired values were found and
simply get the `value` from the returned `std::optional` directly.** This means
that most of the times you will see lines like this

```cpp
const auto value = pidHandler.getAlgoType("someAlgo").value();
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that the exception messages are very cryptic:

terminate called after throwing an instance of 'std::bad_optional_access'
  what():  bad optional access
zsh: IOT instruction (core dumped)  ./a.out

so encouraging not checking is not great. At the same time I think this part is too verbose, we don't need to teach std::optional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I have to reword this a bit. The section here introduces the assumptions we make in the examples below. This should not be read as encouraging using value directly. If we do the proper checks, the examples get rather long.

@@ -0,0 +1,229 @@
# PIDHandler introduction and usage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe somewhere it should say that it's not mandatory to use the PIDHandler and one can iterate manually? For example if each reco particle needs to be accessed once for a given PID algorithm, then it's not necessary to build a map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the particleIDUsed from ReconstructedParticle Direction of inter-object relations
5 participants