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

Update detray version to v0.73.0 #685

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

beomki-yeo
Copy link
Contributor

As of detray v0.73.0, we need to handle the particle hypothesis appropriately.
Free covariance matrix is also removed as it turned out to be unnecessary.

It is not relevant with the detray version update, but the PR make the test for the track parameter estimation cover the positive charge as well.

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.

No fundamental issues of course, just some technical questions.

Would we not need to teach traccc::options about the extension in traccc::finding_config? (And even the one in the simulation config I suppose.) We'd like to be able to set the new particle hypothesis value from the command line as well I guess. 🤔

@beomki-yeo beomki-yeo force-pushed the update-detray-v73 branch 5 times, most recently from 4cfbe68 to 56a1626 Compare August 20, 2024 19:44
@beomki-yeo
Copy link
Contributor Author

Would we not need to teach traccc::options about the extension in traccc::finding_config? (And even the one in the simulation config I suppose.) We'd like to be able to set the new particle hypothesis value from the command line as well I guess

This is done now. As there was no function for casting particle type from double to float, I just used traccc::scalar everywhere but this can be fixed in the future

@beomki-yeo
Copy link
Contributor Author

One issue appeared was that the particle hypothesis may have different charge with the reconstructed track. For example, the track fitting starts with muon hypothesis but there can be tracks with positive charge. In that case, the particle hypothesis is changed to its anti-matter.

@beomki-yeo beomki-yeo force-pushed the update-detray-v73 branch 2 times, most recently from be0f0cc to 500b3a7 Compare August 21, 2024 11:36
@beomki-yeo beomki-yeo force-pushed the update-detray-v73 branch 2 times, most recently from 5b39467 to 0306bf2 Compare August 21, 2024 13:40
@beomki-yeo
Copy link
Contributor Author

Any approval?

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.

Joana is right to worry about all the "if statements". 🤔 We'll have to see exactly what this does to the GPU code, but even when most of the threads will take one path, having any new branching point cannot be good for the code's performance. The question is only how bad it is. Not whether it's bad.

That being said, if we need to live with this, then we do. Still, any "trick" that allows us to perform an operation without an if or switch statement, is a good trick. 😉

@beomki-yeo
Copy link
Contributor Author

I think @krasznaa is misunderstanding sth. The branching only happens inside the function. It is not like it will create entire new branching for the next stages.

@krasznaa
Copy link
Member

I think @krasznaa is misunderstanding sth. The branching only happens inside the function. It is not like it will create entire new branching for the next stages.

?

It's happening inside of a function, which is called from the track finding kernel. This is the definition of branching in the GPU threads... 😕

Again, it may be unavoidable (though maybe not based on the discussion so far), but let's not kid ourselves that adding if(...) statements in device code would be for free...

@beomki-yeo
Copy link
Contributor Author

I have never said that it is free. I just said it is going to be "tiny".
Welp that's why I added the benchmark monitoring. So don't worry too much

@niermann999
Copy link
Contributor

I have never said that it is free. I just said it is going to be "tiny". Welp that's why I added the benchmark monitoring. So don't worry too much

Yeah, its probably benign. It just seemed avoidable

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.

👍

@beomki-yeo beomki-yeo merged commit 1b02e3d into acts-project:main Aug 21, 2024
24 checks passed
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.

3 participants