Insert granularity #771

sebouh137 · 2024-08-20T20:18:25Z

multisegmentation for the insert

Briefly, what does this PR introduce?

Updates the segmentation for the insert so that it has 3 different segmentations:
~12cm2 in layers 1-16
~21 cm2 in layers 17-60 for the left side
~25 cm2 in layers 17-60 for the right side

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue #__)
Documentation update
Other: update on segmentation

Please check if this PR fulfills the following:

Tests for the changes have been added
Documentation has been added / updated
Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

no

Does this PR change default behavior?

yes. different cell sizes in the calorimeter insert. Also a few minor changes: removed thinner front layer from the insert. Added gap between left and right sides. Adjusted the width and heights of the insert slightly.

multisegmentation for the insert

for more information, see https://pre-commit.ci

syntax error corrected

add gap between left and right sides of the detector. This also allows separate segmentations on the left and right sides of the detector

for more information, see https://pre-commit.ci

updated documentation, also use 60 layers instead of 65

for more information, see https://pre-commit.ci

include "side" as an id field

for more information, see https://pre-commit.ci

Force Ecal insert to use "side" as a readout field, so that things don't crash.

fixed issue for subtraction volume.

refactoring the segmentation

…row and column numbers may need to be adjusted by integer offsets, and the margin between the LR edges of the detector and the corners of the hexagons have not been included yet

…gonal tiles

…sert_multiseg

rymilton

Edited some documentation and requested clarification in some parts. Overall it looks good, assuming that the segmentation you introduced behaves as expected like the plots you've shown before.

compact/ecal/forward_insert_homogeneous.xml

compact/hcal/forward_insert.xml

src/InsertCalorimeter_geo.cpp

Co-authored-by: Ryan Milton <[email protected]>

Thanks for catching this Co-authored-by: Ryan Milton <[email protected]>

Co-authored-by: Ryan Milton <[email protected]>

included left_right_gap in the forward_insert.xml

use same left right gap for the forward ecal insert as for the hcal insert (for now).

updated the gap to match the value in the xml

sebouh137 · 2024-08-24T03:40:34Z

Edited some documentation and requested clarification in some parts. Overall it looks good, assuming that the segmentation you introduced behaves as expected like the plots you've shown before.

thank!

sebouh137 · 2024-08-24T15:43:44Z

@rymilton Thanks for the feedback. If you are satisfied with my changes and responses, please approve. If you request futher changes, please let me know. Thanks

rymilton · 2024-08-24T21:52:35Z

Thanks these changes look good. One more thing is that with your branch, I'm seeing this error when using EICrecon: Error in <TGeoVoxelFinder::SortAll>: Volume EcalEndcapPInsert: Cannot make slices on any axis, but I don't see it when using the main branch of epic. Can you reproduce this, and do you know what's causing this?

sebouh137 · 2024-08-25T04:18:11Z

Thanks these changes look good. One more thing is that with your branch, I'm seeing this error when using EICrecon: Error in <TGeoVoxelFinder::SortAll>: Volume EcalEndcapPInsert: Cannot make slices on any axis, but I don't see it when using the main branch of epic. Can you reproduce this, and do you know what's causing this?

I was getting the same errors, and I found that the fix for it is adding "side" to the fields in the CalorimeterHitsMerger_factory in FHCAL.cc . I am fixing this in eic/EICrecon#1595

    app->Add(new JOmniFactoryGeneratorT<CalorimeterHitsMerger_factory>(
      "HcalEndcapPInsertMergedHits", {"HcalEndcapPInsertRecHits"}, {"HcalEndcapPInsertMergedHits"},
      {
        .readout = "HcalEndcapPInsertHits",
        .fields = {"layer", "slice", "side"},
        .refs = {1, 0},
      },
      app   // TODO: Remove me once fixed
    ));

rymilton · 2024-08-25T20:31:57Z

Alright, sounds good. I'll approve these changes then.

…hold (#1595) ### Briefly, what does this PR introduce? Uses a larger threshold for the distance between the neighboring hits in the insert, due to the new cell size. Also include "side" field (ie, left vs right) in the hit merger algorithm ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [ ] New feature (issue #__) - [ ] Documentation update - [X] Other: change of parameters ### Please check if this PR fulfills the following: - [X] Tests for the changes have been added - [X] Documentation has been added / updated - [X] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? Probably. Including the detector "side" as a field in the hit merger algorithm probably breaks the code if using a version of the epic repository from before pull request eic/epic#771 is merged. Therefore this pull request and eic/epic#771 should be merged at the same time as one another ### Does this PR change default behavior? yes. Changes the parameters of the topo clustering and also uses the "side" field in the hit merger. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Dmitry Kalinkin <[email protected]>

sebouh137 and others added 2 commits August 20, 2024 16:18

Update forward_insert.xml

3e8404d

multisegmentation for the insert

[pre-commit.ci] auto fixes from pre-commit.com hooks

d838546

for more information, see https://pre-commit.ci

github-actions bot added topic: forward Positive-rapidity detectors (hadron-going side) topic: calorimetry labels Aug 20, 2024

sebouh137 changed the title ~~Update forward_insert.xml~~ Insert granularity Aug 20, 2024

sebouh137 and others added 11 commits August 20, 2024 16:23

Update forward_insert.xml

cbd796c

syntax error corrected

Update InsertCalorimeter_geo.cpp

8d9c2ef

add gap between left and right sides of the detector. This also allows separate segmentations on the left and right sides of the detector

[pre-commit.ci] auto fixes from pre-commit.com hooks

6057919

for more information, see https://pre-commit.ci

Update forward_insert.xml

5042998

updated documentation, also use 60 layers instead of 65

[pre-commit.ci] auto fixes from pre-commit.com hooks

1fcbc36

for more information, see https://pre-commit.ci

Update InsertCalorimeter_geo.cpp

8211aed

Update InsertCalorimeter_geo.cpp

c83af09

[pre-commit.ci] auto fixes from pre-commit.com hooks

e480086

for more information, see https://pre-commit.ci

Update InsertCalorimeter_geo.cpp

24708f4

[pre-commit.ci] auto fixes from pre-commit.com hooks

401440a

for more information, see https://pre-commit.ci

Update forward_insert.xml

effcf93

include "side" as an id field

sebouh137 requested a review from rymilton August 20, 2024 23:21

sebouh137 and others added 4 commits August 20, 2024 19:27

Update InsertCalorimeter_geo.cpp

2f16451

[pre-commit.ci] auto fixes from pre-commit.com hooks

44e1912

for more information, see https://pre-commit.ci

Update forward_insert_homogeneous.xml

386029f

Force Ecal insert to use "side" as a readout field, so that things don't crash.

Update InsertCalorimeter_geo.cpp

78a87a4

fixed issue for subtraction volume.

sebouh137 marked this pull request as ready for review August 21, 2024 00:57

sebouh137 added 6 commits August 21, 2024 14:09

Update forward_insert.xml

3ba7d8a

refactoring the segmentation

updated the segmentations to update what I have in my layouts. note: …

3b2e54e

…row and column numbers may need to be adjusted by integer offsets, and the margin between the LR edges of the detector and the corners of the hexagons have not been included yet

include a gap between the edge of the PCB and the corners of the hexa…

0b1b090

…gonal tiles

Merge branch 'main' into insert_multiseg

b3a6a6f

removed front plate of insert

821b11c

Merge branch 'insert_multiseg' of https://github.com/eic/epic into in…

20cf60a

…sert_multiseg

sebouh137 enabled auto-merge August 22, 2024 18:21

changed number of hg layers from 20 to 16

96ba413

changed number of hg layers from 20 to 16

ae3102b

rymilton requested changes Aug 23, 2024

View reviewed changes

sebouh137 and others added 4 commits August 23, 2024 21:09

Update compact/hcal/forward_insert.xml

0684e3e

Co-authored-by: Ryan Milton <[email protected]>

Update compact/hcal/forward_insert.xml

b9275b7

Thanks for catching this Co-authored-by: Ryan Milton <[email protected]>

Update compact/hcal/forward_insert.xml

0ae4099

Co-authored-by: Ryan Milton <[email protected]>

Update compact/hcal/forward_insert.xml

0a28d1d

Co-authored-by: Ryan Milton <[email protected]>

sebouh137 disabled auto-merge August 24, 2024 01:10

sebouh137 added 3 commits August 23, 2024 23:28

Update forward_insert.xml

df4a0e5

included left_right_gap in the forward_insert.xml

Update forward_insert_homogeneous.xml

34483a4

use same left right gap for the forward ecal insert as for the hcal insert (for now).

Update InsertCalorimeter_geo.cpp

0e9d93c

updated the gap to match the value in the xml

Merge branch 'main' into insert_multiseg

6464ff2

sebouh137 enabled auto-merge August 24, 2024 03:43

sebouh137 requested a review from rymilton August 24, 2024 18:39

sebouh137 mentioned this pull request Aug 25, 2024

HcalEndcapPInsertImagingProtoClusters: use a larger hit spacing threshold eic/EICrecon#1595

Merged

7 tasks

rymilton approved these changes Aug 25, 2024

View reviewed changes

sebouh137 added this pull request to the merge queue Aug 25, 2024

Merged via the queue into main with commit 5a7dd05 Aug 25, 2024
115 checks passed

sebouh137 deleted the insert_multiseg branch August 25, 2024 23:13

veprbl added this to the 24.09.0 milestone Aug 27, 2024

rymilton mentioned this pull request Nov 8, 2024

Volume EcalEndcapPInsert: Cannot make slices on any axis #781

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert granularity #771

Insert granularity #771

sebouh137 commented Aug 20, 2024 •

edited

Loading

rymilton left a comment

sebouh137 commented Aug 24, 2024

sebouh137 commented Aug 24, 2024

rymilton commented Aug 24, 2024

sebouh137 commented Aug 25, 2024

rymilton commented Aug 25, 2024

Insert granularity #771

Insert granularity #771

Conversation

sebouh137 commented Aug 20, 2024 • edited Loading

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

rymilton left a comment

Choose a reason for hiding this comment

sebouh137 commented Aug 24, 2024

sebouh137 commented Aug 24, 2024

rymilton commented Aug 24, 2024

sebouh137 commented Aug 25, 2024

rymilton commented Aug 25, 2024

sebouh137 commented Aug 20, 2024 •

edited

Loading