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

Return error state if PDG number is unknown #861

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

stephenswat
Copy link
Member

@stephenswat stephenswat commented Feb 12, 2025

Currently, we assume that any unknown PDG number is a muon, but this is obviously false. In order to remove this silent failure state, we should instead return some kind of invalid PDG particle. For this, we can use the same standard that ACTS uses, e.g. to use the number 0.

@stephenswat stephenswat added bug Something isn't working edm Changes to the data model labels Feb 12, 2025
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I'm a little afraid of moving the point where the application encounters an error, very far from where we detect this error. 🤔 How do we expect applications to behave with an unknown particle?

@@ -41,7 +41,8 @@ particle_from_pdg_number(const int pdg_num) {
return detray::pion_minus<scalar_t>();
}

return detray::muon<scalar_t>();
// TODO: Replace with `detray::invalid` in the future
return detray::pdg_particle<scalar_t>(0, 0.f, 0.f);
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect to reach this "legitimately"? I wonder if we should "error out" more aggressively.

Since this code may be used in both host and device code (Though I doubt that device code actually calls this. Does it?), throwing an exception is not on the table. But maybe something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is e.g. reached when we read Geant4 particle files which contain all kinds of horrendous PDG numbers (e.g. 1000701630). Currently our code assumes that such particles are muons by default, which leads to all kinds of hard-to-debug crashes when the sign of $q$ and the sign of $\frac{q}{p}$ differ.

I agree that it would be nice to have a more explicit system of errors, but indeed exceptions are a no-go, so I can't think of a better way than returning an explicitly invalid particle. Remember that the current code doesn't indicate any error at all and just keeps running silently!

@beomki-yeo
Copy link
Contributor

This makes sense to me if there is no particle with pdg number of 0 🤔

@stephenswat
Copy link
Member Author

This makes sense to me if there is no particle with pdg number of 0 🤔

As far as I can tell there isn't: https://pdg.lbl.gov/2007/reviews/montecarlorpp.pdf

ACTS also uses 0 to represent invalid states:

https://github.com/acts-project/acts/blob/36cc0cb978e6e797f840ba2560bd18cd2daa92fc/Core/include/Acts/Definitions/PdgParticle.hpp#L17

Currently, we assume that any unknwon PDG number is a muon, but this is
obviously false. In order to remove this silent failure state, we should
instead return some kind of invalid PDG particle. For this, we can use
the same standard that ACTS uses, e.g. to use the number 0.
@stephenswat stephenswat merged commit 2b31dfb into acts-project:main Feb 14, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working edm Changes to the data model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants