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!: make material validity checks and construction explicit #3494

Merged
merged 23 commits into from
Sep 5, 2024

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Aug 7, 2024

By having the validity check explicit and forbid construction with type conversation, we avoid common pitfalls for the user[1].

The AverageMaterials had to be reworked a bit, since we were doing a lot of implicit type conversions in there. On different compilers we got different results. Now it should be stable.

Thanks also to @paulgessinger for sending me in the correct direction.

[1] The user is me, trying to debug for hours why I get always vacuum material, which could have been caught earlier, by not allowing implicit conversions.

@AJPfleger AJPfleger added this to the next milestone Aug 7, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins labels Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

📊: Physics performance monitoring for 08c7974

Full contents

physmon summary

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

I like

Core/include/Acts/Material/Material.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Material/MaterialSlab.hpp Outdated Show resolved Hide resolved
Core/src/Material/Interactions.cpp Outdated Show resolved Hide resolved
@AJPfleger AJPfleger marked this pull request as ready for review August 8, 2024 15:26
@AJPfleger AJPfleger changed the title fix: make material validity checks explicit fix: make material validity checks and initialisation explicit Aug 8, 2024
@AJPfleger AJPfleger changed the title fix: make material validity checks and initialisation explicit fix: make material validity checks and construction explicit Aug 8, 2024
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

There's a lot of static_casting going on now, do we need all of this? Does going to double consistently give other issues?

Core/src/Material/AverageMaterials.cpp Outdated Show resolved Hide resolved
@AJPfleger
Copy link
Contributor Author

There's a lot of static_casting going on now, do we need all of this? Does going to double consistently give other issues?

I think for this PR, I could be fine to remove the static_cast for the cases float->double and go back to the implicit conversion. But double->float I would love to have there explicitly.

@paulgessinger
Copy link
Member

@AJPfleger Ok

@AJPfleger
Copy link
Contributor Author

What should be the next steps in this PR?

@andiwand
Copy link
Contributor

I think the current version classifies as breaking. As such we would have to wait for the next major version bump to bring this in. Alternative to this is to keep the existing functionality and mark it as deprecated

@AJPfleger AJPfleger changed the title fix: make material validity checks and construction explicit fix!: make material validity checks and construction explicit Aug 14, 2024
@AJPfleger AJPfleger modified the milestones: next, v37.0.0 Aug 14, 2024
@andiwand andiwand mentioned this pull request Aug 29, 2024
@AJPfleger AJPfleger marked this pull request as draft August 30, 2024 14:40
@AJPfleger AJPfleger marked this pull request as ready for review August 31, 2024 17:24
Copy link

sonarcloud bot commented Sep 5, 2024

@kodiakhq kodiakhq bot merged commit 6091312 into acts-project:main Sep 5, 2024
42 checks passed
@github-actions github-actions bot removed the automerge label Sep 5, 2024
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Sep 5, 2024
@AJPfleger AJPfleger deleted the slab-that-material branch September 5, 2024 06:59
@andiwand
Copy link
Contributor

andiwand commented Sep 5, 2024

needs fixes in athena

@andiwand
Copy link
Contributor

andiwand commented Sep 5, 2024

false alarm the failure seems to come from another PR

@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Fails Athena tests This PR causes a failure in the Athena tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants