FastCaloSim: Preparing to make calo det descr immutable.

Currently, the calo detector description in CaloDetDescrManager first gets created, with alignments applied subsequently. In particular, this implies that if one retrieves the detector description in initialize, one will get it without alignments. This is potentially confusing, is bad for MT, and commonly leads to the tracking geometry being incorrectly built without alignments. Therefore, we are trying to move towards building the detector description once, with alignments applied at that point. See ATR-19685 for further discussion.

This means that one should not retrieve CaloDetDescrManager during initialize(). Here, it was being used to initialize the call maps in BasicCellBuilderTool. Modified so that this instead gets triggered via a callback after the aligned calo det descr is created.

However, this exposes some additional points that warrant discussion.

The code here wants to deal with a grid of cells. Whenever one is doing that, it's important to consistently use the unaligned geometry; that is, what one gets from the _raw() functions of the DDEs. The code here wasn't doing that. However, because it was reading the detector description during initialize(), before alignments were applied, what it was getting was mostly the _raw() values --- though with an imporant exception. Even in an unaligned geometry, there's a difference between eta() and eta_raw() in the endcap, due to the 6cm shift of the cryostats from the original design. The code now uses the _raw() functions to build the maps and hence consistently does it in the calo cell coordinate system, but that's a difference from what was done before, and hence results can change in the endcap. I also adjusted the lookup in m_celllist_maps in FastShowerCellBuilderTool::process_particle to adjust to the calorimeter cell reference frame, but there are likely other places where there are still inconsistencies.

This merge request affects 1 package:

• Simulation/FastShower/FastCaloSim

Adding @jchapman ,@vpascuzz as watchers

added Simulation master review-pending-level-1 labels

CI Result SUCCESS

	Athena	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-23685-2019-05-24-02-37
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 38918]

Hi @ssnyder,

John pointed me to this merge request. At least when this part of the code for FastCaloSim was written almost 10 years ago, the .eta() and .phi() values were returning the misaligned cell positions and the simulation relies on these positions to be the ones after misalignment. Changing them now to the _raw values might cause unwanted physics deviations. The code is not using a rigid grid for cell lookup itself, it rather uses a rigid grid to return a vector of cells which are around some logical point on the calo: https://gitlab.cern.ch/atlas/athena/blob/4e9ae25e2ae276e2475d46b4404fac3e128550dd/Simulation/FastShower/FastCaloSim/src/FastShowerCellBuilderTool.cxx#L1720 and then loops over the cells in this vector. The grid was internally already using something like _raw positions for the lookup in a logical grid, but then the cells that were filled in should have consistently used the misaligned positions. Since misalignment is small and locally approximately constant, the change to the _raw value that was added in the MR and the later correction of the lookup during the simulation is possibly doing the same now, but it's a possible dangerous change and not needed in order to correctly deal with misalignment.

Having the initialisation of the geometry now after the misaligment is applied is for sure good. I guess in the athena releases of the last years we never got a negative physics effect from initializing too early because it was just building the lookup list of cells from the perfectly aligned cells, but later during simulation the misaligment was applied and then the correct positions were used. Hence the lookup lists were build without alignment shifts, but since these shits are fortunately small and the lookup lists were build with a too large radius for safety we were lucky and never got a physics effect so far from the missing misalignment in the initialization.

Cheers, Michael

changed the description

added review-approved label and removed review-pending-level-1 label

approved this merge request

Discussion shows no issue in the Merge Request, hence approving. L1 Shift

added review-pending-expert label and removed review-approved label

We want to think about this a bit more from the Simulation side before it is merged.

Cheers,

John

Dear @ssnyder ,

in order to avoid redoing detailed physics validation of the FastCaloSim geometry and object placement, could you please undo the change from eta() to eta_raw() and phi() to phi_raw() or at least make it optional so we can check the difference?

Many thanks, Michael

added review-user-action-required label

removed review-pending-expert label

hi Michael -

Thanks for the feedback. Yeah, i can break this up into two commits. I'll adjust this MR to have just the part that defers retrieving CaloDetDescr, and keep the alignment adjustments in a separate commit.

A warning: when i do that, i get warnings from the unit test like:

-ToolSvc.tool1     WARNING empty map cell EM: eta=-4.85 phi=-2.21405
-ToolSvc.tool1     WARNING empty map cell EM: eta=-4.85 phi=-1.82135
-ToolSvc.tool1     WARNING empty map cell EM: eta=-4.85 phi=-1.33048
-ToolSvc.tool1     WARNING empty map cell EM: eta=4.85 phi=-2.21405
-ToolSvc.tool1     WARNING empty map cell EM: eta=4.85 phi=-1.82135
-ToolSvc.tool1     WARNING empty map cell EM: eta=4.85 phi=-1.33048
-ToolSvc.tool1     WARNING empty map cell EM fine: eta=-2.7125 phi=-3.13444
-ToolSvc.tool1     WARNING empty map cell EM fine: eta=-2.7125 phi=-3.03627

which IIRC was why i was looking at alignment issues in the code in the first place.

added 284 commits

• 4e9ae25e...24da9f0d - 280 commits from branch atlas:master
• 4a7c7eee - FastCaloSim: Preparing to make calo det descr immutable.
• ef887cd4 - Remove alignment corrections.
• 49b4877e - Fix conflicts.
• 657bb1c4 - sync

Compare with previous version

As discussed, this MR was edited to remove the alignment adjustments, and leave the bits implementing deferred retrieval of CaloDetDescr.

As mentioned earlier, this does lead to new warnings from the unit test.

The remaining changes can be found (for now anyway) here: ssnyder/athena@aba41a5d

This merge request affects 1 package:

• Simulation/FastShower/FastCaloSim

Adding @jchapman ,@vpascuzz as watchers

added review-pending-level-1 label and removed review-user-action-required label

CI Result SUCCESS

	Athena	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-23685-2019-06-01-01-32
Athena: number of compilation errors 0, warnings 1
AthSimulation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 39296]

Marked to be squashed and approving.

Tadej (L2)

added review-approved label and removed review-pending-level-1 label

merged

mentioned in commit d92c6e8e

added sweep:ignore label

mentioned in merge request !35285 (merged)

mentioned in merge request !37283 (merged)