Added implementation for hexagonal segmentation and also staggered square segmentations

[sebouh137]

Implemented segmentation variations with hexagonal and square cells, with and without staggering. See arXiv:2308.06939

BEGINRELEASENOTES

• DDSegmentation: added segmentation HexGrid for hexagonal segmentation
• DDSegmentation: added CartesionGridXYStaggered for rectangular segmentation that can be staggered for every other layer

ENDRELEASENOTES

[andresailer]

Hi @sebouh137 , many thanks for your contribution!

I am wondering if we could merge the SquareGrid with the existing CartesionGrid segmentation implementation?

Also it should be possible to use the "MultiSegmentation" and different offsets to achieve the same result as with the staggered SquareGrid?

Finally some example XML snippets would be good, a test that uses the hexgrid segmentation even better.

Thanks again!

[github-actions]

Test Results

       6 files         6 suites   7h 33m 9s ⏱️
   355 tests    355 ✔️ 0 💤 0 ❌
1 055 runs  1 055 ✔️ 0 💤 0 ❌

Results for commit 87689b9.

♻️ This comment has been updated with latest results.

[andresailer]

Hi @sebouh137 Any thoughts on what I proposed before #1161 (comment) ?

[sebouh137]

Hi @andresailer.

Sorry for the delay. Here are my thoughts about your comments. I'll get around to implementing these changes either by the end of this week or early next week.

"I am wondering if we could merge the SquareGrid with the existing CartesionGrid segmentation implementation?"

I agree that would be much simpler. When I was writing this code and testing it for a paper, I could have modified the CartesianGrid segmentation code, but it seemed easier at the time to write a new class as a plugin rather than re-compile the whole dd4hep code (especially since dd4hep was pre-compiled inside a container). But yes it would be possible to replace the SquareGrid with an implementation of a staggering option in CartesianGrid, separately for x, y and z as appropriate.

"Also it should be possible to use the "MultiSegmentation" and different offsets to achieve the same result as with the staggered SquareGrid?"
I tried setting the offset fields in CartesianGrid to be a formula like offset_x="side_length*(layer%2)" in the xml file, but that caused work. Since I couldn't find any documentation on how to do that in the xml file, I ended up writing a segmentation class of my own. I think adding staggering options for CartesianGrid would make more sense

"Finally some example XML snippets would be good, a test that uses the hexgrid segmentation event better."
That is a good idea.

Thanks again!

[sebouh137]

Dear @andresailer,
I found where the problem with the check workflows failed this morning. The test-suite must not have defined a field called "layer". So I modified my code so that if staggering is not enabled, it will not attempt to find the value of "layer" (or whatever stagger keyword is provided). Please rerun the check workflows again now that I've fixed this.
Thanks, Sebouh

[sebouh137]

Dear @andresailer ,
I am not sure what exactly is going on here, and I would like more details of what it's checking so that I can narrow down whatever I need to change in order to make it pass. All of the tests pass, except for the four below. What is it taking the checksum of, and what reference is it comparing them to?
Thanks,
Sebouh

The following tests FAILED:
22 - t_CLICSiD_check_checksum_EcalBarrel (Failed)
23 - t_CLICSiD_check_checksum_full (Failed)
175 - t_MiniTel_check_checksum_Minitel3 (Failed)
176 - t_MiniTel_check_checksum_full (Failed)
Errors while running CTest

[andresailer]

Hi @sebouh137,

The checksum is a validity check for detector models to ensure that a detector model still looks exactly the same when changes are made in DD4hep or how DD4hep is compiled, for example when enabling GEANT4_UNITS. This checksum includes the segmentations, so with the modifications this is somehow changed.

Is is probably OK to just update the expected checksums, but maybe @MarkusFrankATcernch can comment on this?

[wdconinc]

The segmentation description in the checksum enumerates all parameters,

DD4hep/DDCore/src/plugins/DetectorChecksum.cpp

Line 897 in 094aa6c

for ( const auto& v : p ) {

.

That is likely the cause for the change in checksum since new parameters were added to CartesianGridXY.

[MarkusFrankATcernch]

So you changed the segmentation and consequently the checksum changed. No ?

This is in principle the expected behavior.
It is another question what should actually part of the checksum.
I normally believe that the segmentations should be part of them, because they actually are
highly related to the readout geometry (sort of the segmentation approach in Geant4).
This readout geometry is fundamental to a sub-detector and cannot be that easily changed either.
But maybe we want to have this configurable....

In principle the dump shows the various contributions to the checksum:

  +++ 1                                                             +readout  a54220f77daa9eed +iddesc    8a34357f3cc91ee3 +segment  85d8e05343f2c736
  +++ 1    Minitel2                             de 26ddb5c697d5a3f2 +place    b1216132567d8f10 +daughters cbf29ce484222325 +children 52cc8ff6b960a770
  +++ 1                                                             +readout  8b859f9d89006082 +iddesc    53089002050c5a70 +segment  4446d2a5bd91f515
  +++ 1    Minitel3                             de a381e53d77452f9a +place    8461628938ce25d0 +daughters cbf29ce484222325 +children 30b6d41e344e570d
  +++ 1                                                             +readout  3edc69afe0b565c5 +iddesc    a0a7efb65254fea1 +segment  40ff023e9838d7b2
  +++ 0    world                                de b49f13e86137f0ac +place    18ff43e2fb898b14 +daughters cbf29ce484222325 +children bf1b5030a49d8dac

Are your changes only affecting the readout, idddesc and segment part of the detector elements?

[sebouh137]

Dear @MarkusFrankATcernch @wdconinc and @andresailer ,
This is correct that I have added new parameters to CartesianGridXY. The new parameters are options to enable "staggering" (ie, having a cyclical layer-dependent offset in x and y). By default, staggering would is disabled, in order to avoid messing up existing geometries. Further, these parameters are set as optional, so that no errors would occur from having them missing in such existing geometries.
Thanks and best regards,
Sebouh Paul.

[MarkusFrankATcernch]

@sebouh137 @andresailer @wdconinc
From all I can see, there is no way to have a backwards compatible solution to this problem.
If we require backwards compatibility for the checksums, then the current changes to the XY segmentations
must move to the a hexagonal segmentation implementation.
The call

      /// Access to all parameters
      virtual Parameters parameters() const;

in DDSegmentation::Segmentation.h returns all parameters - not just the relevant subset...
there are only two possibilities:

• Segmentations are not part of the checksum (probably also not readouts and IDDescriptors, as they are related. (not backwards compatible)
• Hexagonal segmentations may inherit from CartesianGridXY, but are a seperate implementation. This way existing CartesianGridXY segmentations stay intact. This could probably be made backwards compatible.

Both is not really satisfactory, but I see no other way.

[sebouh137]

Dear @MarkusFrankATcernch ,
In that case, I will leave HexGrid as I have implemented it. I will remove changes from CartesianGridXY, and create a separate class "CartesianGridXYStaggered", which would have all of the staggering. This should allow everything to be backwards compatible.

[MarkusFrankATcernch]

@sebouh137
I know that code duplication is uncool, but I believe this is unfortunately the only way.

[sebouh137]

Dear @MarkusFrankATcernch ,
In that case, I will leave HexGrid as I have implemented it. I will remove changes from CartesianGridXY, and create a separate class "CartesianGridXYStaggered", which would have all of the staggering. This should allow everything to be backwards compatible.

I have now implemented this change. You can run the checks again now.
Best regards,
Sebouh

[sebouh137]

Dear @andresailer,
I see where this is failing to build. While editing my files, I had forgotten to add "CartesianGridXYStaggered::" to the cellDimensions method in CartesianGridXYStaggered.cpp. I think it should work now.
Best regards,
Sebouh

[andresailer]

Dear @andresailer, I see where this is failing to build. While editing my files, I had forgotten to add "CartesianGridXYStaggered::" to the cellDimensions method in CartesianGridXYStaggered.cpp. I think it should work now. Best regards, Sebouh

Actually I forgot the CartesianGridXYStaggered:: in dccd89f , but the function declaration was missing before 😄

Thanks for adding the missing piece!

[sebouh137]

Dear @andresailer ,
All of the changes you have requested have been marked as resolved, but it is still showing "one change requested". I can't see any non-resolved change on here. Further, all of the checks have now passed. Let me know if there's anything else that you need on my end.
Thanks,
Sebouh

[andresailer]

Thanks again for your contribution!