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: GeoShapes to DetectorVolumes and GeoMaterial Converters #3268

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

Matthewharri
Copy link
Contributor

@Matthewharri Matthewharri commented Jun 7, 2024

This PR introduces a converter to convert geoShapes to DetectorVolumes. Some of the shapes added are very simple converters, i.e. simple bounding boxes which will probably be changed later on. I also bump the GeoModel version from 4.6.0 to 6.0.0 since Athena is using a much newer version. I also change (with the help of @paulgessinger) how cmake looks for the system installation of GeoModel.

I also include a way to convert GeoMaterial to Acts::Material

@junggjo9

blocked by

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Component - Plugins Affects one or more Plugins labels Jun 7, 2024
@Matthewharri Matthewharri changed the title Feat: Convert geoShapes to DetectorVolumes feat: Convert geoShapes to DetectorVolumes Jun 7, 2024
Copy link

github-actions bot commented Jun 7, 2024

📊: Physics performance monitoring for 7608689

Full contents

physmon summary

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.31%. Comparing base (859e7af) to head (6b016ca).
Report is 43 commits behind head on main.

Current head 6b016ca differs from pull request most recent head cd12571

Please upload reports for the commit cd12571 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3268      +/-   ##
==========================================
- Coverage   47.37%   47.31%   -0.06%     
==========================================
  Files         511      512       +1     
  Lines       30249    30437     +188     
  Branches    14677    14795     +118     
==========================================
+ Hits        14329    14400      +71     
- Misses       5386     5404      +18     
- Partials    10534    10633      +99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AJPfleger AJPfleger added this to the next milestone Jun 7, 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.

Generally, this seems sensible to me.

  1. Do we have any way to test this in an isolated way in CI?
  2. Medium term, I'll likely want to reuse the volume bounds creation, as that will translate 1:1 into the Gen3 geometry model.

CMakeLists.txt Outdated Show resolved Hide resolved
Plugins/GeoModel/src/GeoModelToDetVol.cpp Outdated Show resolved Hide resolved
Plugins/GeoModel/src/GeoModelToDetVol.cpp Outdated Show resolved Hide resolved
@Matthewharri Matthewharri changed the title feat: Convert geoShapes to DetectorVolumes feat: GeoShapes to DetectorVolumes and GeoMaterial Converters Jul 4, 2024
@Matthewharri
Copy link
Contributor Author

@noemina should be all set now.

noemina
noemina previously approved these changes Aug 2, 2024
@noemina
Copy link
Contributor

noemina commented Aug 2, 2024

@Matthewharri it seems there is still a problem with the compilation?

@andiwand
Copy link
Contributor

andiwand commented Aug 2, 2024

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Aug 2, 2024
@Matthewharri
Copy link
Contributor Author

Apparently there was, but I think it should be fixed now.

@Matthewharri
Copy link
Contributor Author

Ok, So this is now failing because there are warnings in the geomodel compilation, and it is treating those as errors. Any suggestions @noemina or @andiwand

@andiwand
Copy link
Contributor

andiwand commented Aug 2, 2024

Ok, So this is now failing because there are warnings in the geomodel compilation, and it is treating those as errors. Any suggestions @noemina or @andiwand

Looking at the compile command

ccache /usr/bin/c++ -DGeoModelKernel_EXPORTS -I/__w/acts/acts/build/_deps/geomodel-src/GeoModelCore/GeoModelKernel -I/__w/acts/acts/build/_deps/geomodel-src/GeoModelCore/GeoGenericFunctions -isystem /__w/acts/acts/cmake/assert_include -isystem /usr/include/eigen3 -Wall -Wextra -Wpedantic -Wshadow -Wno-unused-local-typedefs -Werror -Wno-deprecated-declarations -O3 -DNDEBUG -std=c++20 -fPIC -MD -MT _deps/geomodel-build/GeoModelCore/GeoModelKernel/CMakeFiles/GeoModelKernel.dir/src/GeoNodeAction.cxx.o -MF _deps/geomodel-build/GeoModelCore/GeoModelKernel/CMakeFiles/GeoModelKernel.dir/src/GeoNodeAction.cxx.o.d -o _deps/geomodel-build/GeoModelCore/GeoModelKernel/CMakeFiles/GeoModelKernel.dir/src/GeoNodeAction.cxx.o -c /__w/acts/acts/build/_deps/geomodel-src/GeoModelCore/GeoModelKernel/src/GeoNodeAction.cxx

It seems that GeoModel is not included using -isystem but just -I which will cause the compiler to raise warnings in the included files. This means GeoModel is not integrated correctly in ACTS via CMake

@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Aug 6, 2024
@andiwand
Copy link
Contributor

andiwand commented Aug 6, 2024

This is blocked by #3476 which is blocked by having an external build for GeoModel

paulgessinger added a commit that referenced this pull request Aug 14, 2024
Bumps GeoModel version and deals with the necessary changes. This is a
subset of #3268 which isolates
the version change and tackles the CI errors faced there.

---------

Co-authored-by: Paul Gessinger <[email protected]>
@github-actions github-actions bot removed the Infrastructure Changes to build tools, continous integration, ... label Aug 19, 2024
@Matthewharri
Copy link
Contributor Author

With #3476 merged in, the issue with the builds is fixed. Is this now good to be merged in? @noemina or @andiwand

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.

A couple of nitpicks from my side. Otherwise looks good to go

@Matthewharri
Copy link
Contributor Author

@andiwand Ok, I addressed your things, so assuming nothing got messed up the CI should be fine, and then this is good to go

@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Aug 19, 2024
andiwand
andiwand previously approved these changes Aug 19, 2024
@andiwand
Copy link
Contributor

should we override sonarcloud in this case @paulgessinger ?

Copy link

sonarcloud bot commented Aug 19, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 25%)
0.0% Line Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

@github-actions github-actions bot added the Stale label Sep 18, 2024
@github-actions github-actions bot removed the Stale label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Plugins Affects one or more Plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants