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

feat: add material conversion + test for TGeo #2496

Merged

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Oct 2, 2023

This is a first PR in a set of a series PRs to establish the ODD in the new Detector schema.

This particular PR adds the possibility to convert material via TGeoMaterial and adds tests for this.
It also cleans ups the TGeoSurface converter slightly.

Note:

  • due to internal unit change of ROOT up to 6.25.01 the unit tests on this Material conversion test is enables only for ROOT versions after this change.

@asalzburger asalzburger requested a review from AJPfleger October 2, 2023 08:42
@asalzburger asalzburger added this to the next milestone Oct 2, 2023
Copy link
Contributor

@AJPfleger AJPfleger left a comment

Choose a reason for hiding this comment

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

In multiple place the factor 2 * is introduced. Why?

Plugins/TGeo/CMakeLists.txt Show resolved Hide resolved
Plugins/TGeo/src/TGeoSurfaceConverter.cpp Outdated Show resolved Hide resolved
@asalzburger asalzburger requested a review from AJPfleger October 2, 2023 09:38
@asalzburger
Copy link
Contributor Author

In multiple place the factor 2 * is introduced. Why?

That was a bug - but this code what not yet used, the half length and length was wrongly matched.
It becomes relevant now as I use the thickness/half thickness for the Material condensing.

@github-actions github-actions bot added the Component - Plugins Affects one or more Plugins label Oct 2, 2023
AJPfleger
AJPfleger previously approved these changes Oct 2, 2023
@asalzburger
Copy link
Contributor Author

It is indeed the TGeoConversion to change output. Checking this in detail now.

@acts-policybot acts-policybot bot dismissed AJPfleger’s stale review October 2, 2023 11:45

Invalidated by push of 4cd855a

@asalzburger
Copy link
Contributor Author

The change happens in
ProtoLayer where we estimate the layer with
double thickness = element->thickness();
Nothings serious, but enough to change output.

I will verify the correct behaviour and if correct, change the references.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #2496 (7158a65) into main (e349434) will not change coverage.
The diff coverage is n/a.

❗ Current head 7158a65 differs from pull request most recent head 572a539. Consider uploading reports for the commit 572a539 to get more accurate results

@@           Coverage Diff           @@
##             main    #2496   +/-   ##
=======================================
  Coverage   49.81%   49.81%           
=======================================
  Files         466      466           
  Lines       26253    26253           
  Branches    12054    12054           
=======================================
  Hits        13077    13077           
  Misses       4606     4606           
  Partials     8570     8570           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Oct 2, 2023
@asalzburger asalzburger requested a review from AJPfleger October 2, 2023 13:01
@asalzburger
Copy link
Contributor Author

Hey @AJPfleger - I have checked that this behaviour is correct now (and have put this information also into the unit tests).

Thus, I updated the references.

@asalzburger asalzburger requested a review from AJPfleger October 2, 2023 13:22
AJPfleger
AJPfleger previously approved these changes Oct 2, 2023
@asalzburger
Copy link
Contributor Author

The tiny differences in the test_examples are expected due to thickness change of the ODD detector

@asalzburger asalzburger force-pushed the feat-dd4hep-pr0-tgeo-update branch from 7158a65 to f460c07 Compare October 2, 2023 19:39
@acts-policybot acts-policybot bot dismissed AJPfleger’s stale review October 2, 2023 19:39

Invalidated by push of f460c07

@asalzburger
Copy link
Contributor Author

Merging this after approval and updating references.

@asalzburger asalzburger merged commit c50a62d into acts-project:main Oct 3, 2023
asalzburger added a commit that referenced this pull request Oct 3, 2023
This PR is part 2 of the odd + detector series:
- it promotes the `LayerStructure::Support` to `ProtoSupport`, just as
done with the `ProtoBinning`.

It sits on top of #2496 and is thus marked blocked.
@paulgessinger paulgessinger modified the milestones: next, v30.1.0 Oct 5, 2023
@osbornjd
Copy link
Contributor

osbornjd commented Nov 6, 2023

@asalzburger After updating from v30.0.0 to v30.3.0 I have found that this PR (and specifically the factor of 2 in the TGeoSurfaceConverter discussed above) leads to an error message in our geometry building that looks like the following:

09:41:23    LayerArrayCr   ERROR     Layers are overlapping at: 367.247. This should never happen. Please check your geometry description.

This happens for all of our TPC layers where we create fake boxes that approximate the pad plane readout layers. It turns out if I remove this factor of 2 from a local build these error messages go away, as is the case in v30.0.0. The really strange part is that the code functions completely normally with v30.3.0, passes all CI and performance checks etc. So the error message is somehow anamolous. I'll find a way to suppress it for now but can you comment further on why this change was made and if we should expect this behavior? It says above that this was a bug but the boxes that we build in TGeo have not had any overlap issues and we have a TGeo overlap check in our CI that runs and it does not warn about any overlaps. Perhaps there is some difference in definition or expectation between the Acts geometry and TGeo geometry that we are somehow mixing up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Plugins Affects one or more Plugins Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants