add ITS sensors to parallel world #13163

maciacco · 2024-05-27T13:43:36Z

@mconcas Adding ITS sensors to parallel world to prioritize navigation of sensitive elements in case of overlapping volumes

github-actions · 2024-05-27T13:43:47Z

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass3
async-2023-pbpb-apass4
async-2022-pp-apass6-2023-PbPb-apass2
async-2022-pp-apass4
async-2022-pp-apass4-accepted
async-2022-pp-apass6-2023-PbPb-apass2-accepted
async-2023-pbpb-apass3-accepted
async-2023-pbpb-apass4-accepted
async-2023-pp-apass4
async-2023-pp-apass4-accepted
async-2024-pp-apass1
async-2024-pp-apass1-accepted
async-2022-pp-apass7
async-2022-pp-apass7-accepted
async-2024-pp-cpass0
async-2024-pp-cpass0-accepted

Please consider the following formatting changes to AliceO2Group#13163

Detectors/gconfig/g4config.in

Co-authored-by: Matteo Concas <mconcas@cern.ch>

mconcas · 2024-05-27T13:48:45Z

@mconcas Adding ITS sensors to parallel world to prioritize navigation of sensitive elements in case of overlapping volumes

Thanks a lot @maciacco , approving to trigger CI.

alibuild · 2024-06-06T15:18:40Z

Error while checking build/O2/fullCI for d6bf6b5 at 2024-08-10 06:05:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/FIT/FT0/simulation/src/Detector.cxx:59:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/CPV/simulation/src/Detector.cxx:53:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/PHOS/simulation/src/Detector.cxx:61:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/TOF/simulation/src/Detector.cxx:49:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/ITSMFT/MFT/simulation/src/DigitizerTask.cxx:36:16: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/GlobalTracking/src/MatchGlobalFwd.cxx:844:17: error: use '= default' to define a trivial default constructor [modernize-use-equals-default]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

github-actions · 2024-08-10T01:03:56Z

This PR did not have any update in the last 30 days. Is it still needed? Unless further action in will be closed in 5 days.

alibuild · 2024-08-13T04:23:07Z

Error while checking build/O2/fullCI for 1b3d7e0 at 2024-08-15 14:01:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/FIT/FT0/simulation/src/Detector.cxx:59:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/CPV/simulation/src/Detector.cxx:53:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/PHOS/simulation/src/Detector.cxx:61:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/TOF/simulation/src/Detector.cxx:49:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/ITSMFT/MFT/simulation/src/DigitizerTask.cxx:36:16: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/GlobalTracking/src/MatchGlobalFwd.cxx:844:17: error: use '= default' to define a trivial default constructor [modernize-use-equals-default]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

alibuild · 2024-08-20T17:25:29Z

Error while checking build/O2/fullCI for ebe61e9 at 2024-08-24 06:52:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/FIT/FT0/simulation/src/Detector.cxx:59:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/PHOS/simulation/src/Detector.cxx:61:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/CPV/simulation/src/Detector.cxx:53:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/TOF/simulation/src/Detector.cxx:49:11: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/ITSMFT/MFT/simulation/src/DigitizerTask.cxx:36:16: error: use '= default' to define a trivial destructor [modernize-use-equals-default]
/sw/SOURCES/O2/13163-slc8_x86-64/0/Detectors/GlobalTracking/src/MatchGlobalFwd.cxx:844:17: error: use '= default' to define a trivial default constructor [modernize-use-equals-default]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

sawenzel · 2024-08-21T14:36:59Z

DataFormats/Detectors/Common/include/DetectorsCommonDataFormats/AlignParam.h


  /// apply object to geoemetry
-  bool applyToGeometry() const;
+  bool applyToGeometry(bool usePW = false, bool addSensorPW = false, bool addMetalPW = false, bool addChipPW = false) const;


In principle this function has nothing to do with ITS. Yet, we pass several ITS specific arguments to it.
Could we think of a cleaner structure? Something like passing a AlignGeomOptions struct (which is then extensible)? The struct could contain ITS specific members.

This has been changed in the latest version. Now the ITS-specific configuration is handled by the ITS implementation of the o2::Detector::fillParallelWorld function

sawenzel · 2024-08-21T14:39:36Z

DataFormats/Detectors/Common/src/AlignParam.cxx

+  std::string pathStr;
+  if (path) {
+    pathStr.append(path);
+    if (getDetFromSymName(getSymName()) == "ITS" && (pathStr.find("ITSUChip") != std::string::npos) && usePW) {


from a framework point of view, this looks is a bit unfortunate. We go from something generic to something that contains ITS specific code. Could we think of alternatives, in which we call detector spefific routines adjustments (callback, virtual function, etc.)?

in the new implementation the volumes are added to the parallel world in a second step after the alignment. As in the other comment, the ITS code is now handling the filling of ITS volumes

Detectors/Base/include/DetectorsBase/GeometryManagerParam.h

sawenzel · 2024-08-21T14:42:51Z

Nice progress... and sorry for my late comments. I just realized that the adjustments are not done within ITS geometry code but in the global alignment framework. Not sure if there are alternatives ... but it might make sense to shortly think about if we can do it slightly better.

mconcas · 2024-08-26T11:22:55Z

Nice progress... and sorry for my late comments. I just realized that the adjustments are not done within ITS geometry code but in the global alignment framework. Not sure if there are alternatives ... but it might make sense to shortly think about if we can do it slightly better.

Now that you pointed this out, I realize that we can definitely do it in a more generic way.
We'll re-iterate on this PR. Thanks for the review.

Please consider the following formatting changes to AliceO2Group#13163

maciacco · 2024-09-09T15:08:01Z

Dear @sawenzel I factorized the code so that the parallel world is now filled after the alignment. This way there is no issue in the export of the geometry to file + the filling of the parallel world is handled separately by each detector (only ITS as of now) avoiding the previous clashes with the alignment framework. This required to add a new function in the base Detector class, which is overridden only for ITS. Could you please have a look to the latest version? Thank you!!

alibuild · 2024-09-10T06:33:31Z

Error while checking build/O2/fullCI for ca0a024 at 2024-09-25 13:56:

## sw/BUILD/Rivet-latest/log
make[2]: *** [Makefile:544: core.cpp] Error 127
make[1]: *** [Makefile:440: all-recursive] Error 1
make: *** [Makefile:561: all-recursive] Error 1

Full log here.

sawenzel · 2024-09-20T11:23:53Z

I will take a look at the latest restructuring soon. For now, I would like to suggest trying out the new improvements in ROOT, available in tag v6-32-04-alice2 of our ROOT (https://github.com/alisw/root/releases/tag/v6-32-04-alice2).

You need to enable these features like so:

 pw = gGeoManager->CreateParallelWorld("priority_its_sensor");
 if (getenv("TGEO_PW_USEBVH")) {
      pw->SetAccelerationMode(TGeoParallelWorld::AccelerationMode::kBVH);
    }
 if (getenv("TGEO_PW_CACHING")) {
      TGeoNavigator::SetPWSafetyCaching(true);
 }

I would be good to get an update on hit improvements, CPU runtime, memory. I personally found that adding just the ITS sensor gave the best improvements in hit numbers.
Once everything confirmed, we can put this into production.

maciacco · 2024-09-20T11:50:37Z

Dear @sawenzel thanks for the updates and for the work on the ROOT side! We will update you soon on the simulation performance using the latest version.

Detectors/Base/src/Detector.cxx

sawenzel

Looking good to me apart from some minor cleanup requests (see inline comments)

mconcas and others added 6 commits May 27, 2024 15:35

Add Parallel world in ITS aligner

739e494

toggle parallel world (AliceO2Group#82)

70a4c6a

Don't close geomeitry here

3241cf0

qwer

92fc6f9

add ITS sensors to PW

50a4e61

remove debug print

3ba9e9c

maciacco requested review from a team, benedikt-voelkel, sawenzel and shahor02 as code owners May 27, 2024 13:43

alibuild and others added 2 commits May 27, 2024 13:44

Please consider the following formatting changes

2bf26f4

Merge pull request #1 from alibuild/alibot-cleanup-13163

e11c643

Please consider the following formatting changes to AliceO2Group#13163

mconcas reviewed May 27, 2024

View reviewed changes

Detectors/gconfig/g4config.in Outdated Show resolved Hide resolved

Update Detectors/gconfig/g4config.in

d6bf6b5

Co-authored-by: Matteo Concas <mconcas@cern.ch>

mconcas approved these changes May 27, 2024

View reviewed changes

mconcas previously approved these changes May 30, 2024

View reviewed changes

github-actions bot added the stale label Aug 10, 2024

maciacco added 2 commits August 12, 2024 17:52

Merge branch 'AliceO2Group:dev' into its-parallel-world

932e262

disable geometry saving w/ pw + fill pw w/ full list of vols

1b3d7e0

maciacco dismissed mconcas’s stale review via 1b3d7e0 August 12, 2024 16:02

mconcas removed the stale label Aug 20, 2024

Merge branch 'AliceO2Group:dev' into its-parallel-world

ebe61e9

sawenzel reviewed Aug 21, 2024

View reviewed changes

Detectors/Base/include/DetectorsBase/GeometryManagerParam.h Show resolved Hide resolved

make pw filling detector specific (todo: configuration)

3d38760

maciacco marked this pull request as draft September 4, 2024 17:24

maciacco and others added 2 commits September 4, 2024 19:25

fix filling order + add configurables

9494c0c

Please consider the following formatting changes

bcaddf4

alibuild mentioned this pull request Sep 4, 2024

Please consider the following formatting changes to #13163 maciacco/AliceO2#2

Merged

maciacco added 7 commits September 4, 2024 19:27

Merge pull request #2 from alibuild/alibot-cleanup-13163

6c80f2a

Please consider the following formatting changes to AliceO2Group#13163

Merge branch 'AliceO2Group:dev' into its-parallel-world

add5531

remove unused params

6aa192d

remove unused include + add new class to linkdef

1cb77ff

remove print + format string only once

e764530

get param only once

d2a9e9f

remove useless comment

ca0a024

maciacco marked this pull request as ready for review September 9, 2024 10:11

maciacco added 2 commits October 1, 2024 10:47

Merge branch 'AliceO2Group:dev' into its-parallel-world

3f72471

enable pw optimisation

a9fcf2b

sawenzel reviewed Oct 11, 2024

View reviewed changes

Detectors/Base/src/Detector.cxx Show resolved Hide resolved

sawenzel requested changes Oct 11, 2024

View reviewed changes

add empty implementation for generic pw method

ef32328

sawenzel merged commit 6fbc0f9 into AliceO2Group:dev Oct 11, 2024

add ITS sensors to parallel world #13163

add ITS sensors to parallel world #13163

Uh oh!

Conversation

maciacco commented May 27, 2024

Uh oh!

github-actions bot commented May 27, 2024

Uh oh!

Uh oh!

mconcas commented May 27, 2024

Uh oh!

alibuild commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 10, 2024

Uh oh!

alibuild commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alibuild commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sawenzel Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

maciacco Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

sawenzel Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

maciacco Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sawenzel commented Aug 21, 2024

Uh oh!

mconcas commented Aug 26, 2024

Uh oh!

maciacco commented Sep 9, 2024

Uh oh!

alibuild commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sawenzel commented Sep 20, 2024

Uh oh!

maciacco commented Sep 20, 2024

Uh oh!

Uh oh!

sawenzel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

alibuild commented Jun 6, 2024 •

edited

Loading

alibuild commented Aug 13, 2024 •

edited

Loading

alibuild commented Aug 20, 2024 •

edited

Loading

alibuild commented Sep 10, 2024 •

edited

Loading