Skip to content

[FOCAL-66] ECAL geoemetry and step manager for pads#13249

Merged
mfasDa merged 15 commits into
AliceO2Group:devfrom
amatyja:dev
Jul 10, 2024
Merged

[FOCAL-66] ECAL geoemetry and step manager for pads#13249
mfasDa merged 15 commits into
AliceO2Group:devfrom
amatyja:dev

Conversation

@amatyja
Copy link
Copy Markdown
Contributor

@amatyja amatyja commented Jun 26, 2024

[FOCAL-66] ECAL geometry and step manager for pads

@github-actions
Copy link
Copy Markdown
Contributor

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-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0

iarsene
iarsene previously approved these changes Jul 3, 2024
Copy link
Copy Markdown
Collaborator

@mfasDa mfasDa left a comment

Choose a reason for hiding this comment

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

I think the most important change needed is to outsource the Hit processing of the separate subsystems to dedicated functions ProcessHitsEPad, ProcessHitsEPixel, ProcessHitsHCAL, for 2 reasons:
a) The check for minimal energy deposit might shadow other subsystems, if they are in a combined process function
b) We would like to be able to switch off subsystems via the o2::ConfigurableParams class to run ECAL or HCAL alone (i.e. for testbeam simulation)

Otherwise I think the PR is already in a good shape and soon ready to be merged.

Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
@alibuild
Copy link
Copy Markdown
Collaborator

alibuild commented Jul 3, 2024

Error while checking build/O2/fullCI for 547b62e at 2024-07-04 09:30:

## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/13249-slc8_x86-64/0/Detectors/FOCAL/simulation/src/Detector.cxx:980:33: error: use nullptr [modernize-use-nullptr]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

amatyja added 5 commits July 4, 2024 14:49
Added ProcessHItsECAL and ProcessHitHCAL methods.
Split ProcessHit to ProcessHitECAL and ProcssHitHCAL.
…sHits

Added sensitive volume for Pixels. Minor changes in geometry - splitting of Pads and Pixels. Additional method for Pixels responsible for ProcessHits in Pixels.
Copy link
Copy Markdown
Collaborator

@mfasDa mfasDa left a comment

Choose a reason for hiding this comment

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

There is one small issue left:

The vector "mSensitive..." we use to declare all FOCAL volumes as sensitive - in order to build the decay tree and not to have broken chains. For this one single container is sufficient - currently the top volume is declared 3 times as sensitive, for each subsystem. The real sensitive volume, for which hits are created, is anyhow handled differently.

Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
One sensitive volume list instead of three for HCAL, Pads Pixels.
@mfasDa mfasDa self-requested a review July 10, 2024 07:56
mfasDa
mfasDa previously approved these changes Jul 10, 2024
Copy link
Copy Markdown
Collaborator

@mfasDa mfasDa left a comment

Choose a reason for hiding this comment

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

Now looks good and ready to merge.

@mfasDa mfasDa enabled auto-merge (squash) July 10, 2024 08:04
iarsene
iarsene previously approved these changes Jul 10, 2024
auto-merge was automatically disabled July 10, 2024 08:26

Head branch was pushed to by a user without write access

@amatyja
Copy link
Copy Markdown
Contributor Author

amatyja commented Jul 10, 2024

I've added missing string method c_srt() to logs.

Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
Comment thread Detectors/FOCAL/simulation/src/Detector.cxx Outdated
@mfasDa mfasDa self-requested a review July 10, 2024 08:43
mfasDa
mfasDa previously approved these changes Jul 10, 2024
Copy link
Copy Markdown
Collaborator

@mfasDa mfasDa left a comment

Choose a reason for hiding this comment

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

Now OK, ready to merge

@mfasDa mfasDa enabled auto-merge (squash) July 10, 2024 08:49
@mfasDa mfasDa disabled auto-merge July 10, 2024 09:11
@mfasDa mfasDa merged commit 68770ca into AliceO2Group:dev Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants