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

fix: several assertions #917

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Jan 23, 2025

Fixes a couple of assertions:

  • A couple of out-of-bounds assertions (material scan, populator test, inspector, detector scanner, ranges cuda test)
  • don't pass zero standard deviation to std::normal_distribution

@niermann999 niermann999 added bug Something isn't working priority: medium medium priority labels Jan 23, 2025
@niermann999 niermann999 force-pushed the fix-several-assertions branch 3 times, most recently from 934f76f to 28e0ffb Compare January 25, 2025 10:52
@niermann999 niermann999 marked this pull request as ready for review January 25, 2025 12:16
@niermann999 niermann999 force-pushed the fix-several-assertions branch 6 times, most recently from 168e9d6 to a9de44a Compare January 26, 2025 02:19
@@ -52,7 +52,10 @@ struct random_track_generator_config {

/// Track origin
darray<scalar_t, 3> m_origin{0.f, 0.f, 0.f};
darray<scalar_t, 3> m_origin_stddev{0.f, 0.f, 0.f};
darray<scalar_t, 3> m_origin_stddev{
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, what is an advantage of setting default values to epsilon? clients will need to change this to (0,0,0) manually when they want to fix the vertex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My compiler throws an assertion in release mode when 0 is passed as stddev. So I cannot run any test/tool that involves the random track generation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Gaussian distribution is not directly defined for stddev = 0 mathematically, so I guess that the implementation in the standard library calculates the sample value by dividing by the standard deviation somewhere, so that this would result in an FPE? I changed our method now, so that it returns the mean when the standard dev is zero without calling the dist

Copy link
Collaborator

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

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

Now I think the epsilon is a bit weird..

@niermann999 niermann999 force-pushed the fix-several-assertions branch from a9de44a to 067c5c5 Compare January 26, 2025 09:25
@niermann999 niermann999 force-pushed the fix-several-assertions branch from 067c5c5 to 17571ed Compare January 26, 2025 17:07
Copy link

@beomki-yeo beomki-yeo merged commit 961900c into acts-project:main Jan 27, 2025
19 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 priority: medium medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants