AIDASoft / DD4hep

Detector Description Toolkit for High Energy Physics
http://dd4hep.cern.ch
GNU Lesser General Public License v3.0
49 stars 95 forks source link

memory issue for large geometries #1173

Closed saraheno closed 8 months ago

saraheno commented 12 months ago

Hi, I am trying to build a large geometry that has a large number of segments. The code is at https://github.com/saraheno/DualTestBeam and especially the problem I am having is when it is run with the command

ddsim --compactFile=/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/DRFIDEAonly.xml --runType=batch -G --steeringFile /home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCALsteering.py --outputFile=./junk.root --part.userParticleHandler='' -G --gun.position="0.05,0.,-210cm" --gun.direction "0 0 1" --gun.energy "20GeV" --gun.particle="pi-" -N 1

The error message I get is attached below. I think this is a memory issue because when I change the parameter setting the number of elements https://github.com/saraheno/DualTestBeam/blob/8bd355a3b9f6e6ee4fa52c05c80c510e7d924e60/compact/SCEPCALConstants.xml#L28 to half the value I want, I do not get a crash.

Is there something I can do to increase the available memory (assuming it is a code limitation, and not a limitation in my hardware... is there a way to tell which is the problem)?

crash

andresailer commented 12 months ago

Hi @saraheno

Well, it is possible [1] that the memory is somehow limited by the operating system. You could run top in a parallel terminal to see how memory use of ddsim, and free memory of the machine evolves.

[1]: ulimit -m should show the max memory for a process for example.

saraheno commented 11 months ago

so as far as you know, there is no limit inside dd4hep that needs to be expaneded for complicated geometry?

andresailer commented 11 months ago

I am not aware of any limit applied by DD4hep, Geant4, or Root.

BrieucF commented 11 months ago

Dear Sarah, all,

I am experiencing the same kind of issue with a drift chamber implementation: in its current form, it eats up to 10 GB memory (and gets killed e.g. on lxplus). I realized that the guilty part was the usage of IntersectionSolid. If I use something else than this with the exact same number of objects, the memory usage goes down to 1 GB. The problem is that I can not easily represent the complex volume (currently the intersection of a hyperboloid with a rotated tube segment) with 'native' shapes.

To @saraheno, I see that you use SubtractionSolid in DRFiber_geo.cpp and in EdgeDet_geo.cpp. I suspect that the outrageous memory usage comes from there. To confirm this, could you try to replace those shapes by anything else which does not result from a Boolean operation and see how the memory usage behaves?

To all, would there be a way to improve this? Those Boolean operation are very convenient.

Another piece of information is that I have the impression that this explosion of memory usage only occurs when the geometry is translated to Geant4. If I run e.g. geoDisplay on the geometry for visualization purpose only, the memory usage is very reasonable also with the IntersectionSolid.

Thanks in advance!

saraheno commented 11 months ago

@BrieucF Thanks for this information, Brieuc! I have to admit that my knowledge of the geant4 geometry routines is not very advanced. But I'll see if I can find some time to investigate other structures instead. At least this once I find the time should be a productive path!

saraheno commented 10 months ago

so looking at https://geant4-userdoc.web.cern.ch/UsersGuides/ForApplicationDeveloper/html/Detector/Geometry/geomSolids.html
I really don't see a way to make a hollow box without using the boolean operations? You can make hollow tubes, but not hollow boxes? so I think I am stuck with memory problems when using dd4hep?

does G4MultiUnion have the same memory problems as the subtraction? It is also a bool.

or maybe it is possible to use G4TessellatedSolid ? is it known if this has memory problems?

andresailer commented 10 months ago

Hi @saraheno,

Can you specifically explain what your geometry is supposed to look like, where in the code and XML this is described?

I don't see where the constant you point to is used in the code itself.

https://github.com/search?q=repo%3Asaraheno%2FDualTestBeam%20DRFiberNsizeIDEA&type=code

saraheno commented 10 months ago

https://github.com/saraheno/DualTestBeam/blob/35c7e4166827fa040c5ecee69d2388b2adb5afd5/src/DRFiber_geo.cpp#L64

atolosadelgado commented 10 months ago

so looking at https://geant4-userdoc.web.cern.ch/UsersGuides/ForApplicationDeveloper/html/Detector/Geometry/geomSolids.html I really don't see a way to make a hollow box without using the boolean operations? You can make hollow tubes, but not hollow boxes? so I think I am stuck with memory problems when using dd4hep?

does G4MultiUnion have the same memory problems as the subtraction? It is also a bool.

or maybe it is possible to use G4TessellatedSolid ? is it known if this has memory problems?

Hi,

depends on the final goal of the geometry. If you just want a hollow box, a simple approach would be to place a smaller "air box" inside the original box. The mother material is replaced by the daughter material inside the daughter volume. There are other alternatives to boolean operations or tessellated solids;, these must be used only in extreme cases where there is no other alternative.

You may want to ask this question in the Geant4 forum for further support :) https://geant4-forum.web.cern.ch/

Best, Alvaro

saraheno commented 9 months ago

I updated the code to remove all booleans. However, it still seems to crash with memory issues. I put the current errror below. The code is at https://github.com/saraheno/DualTestBeam

It is run with

ddsim --compactFile=/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/DRFSCEPonly.xml --runType=batch -G --steeringFile /home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCALsteering.py --outputFile=/data/users/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/output/out_FSCEPonly_10GeV_e-.root --part.userParticleHandler= -G --gun.position="0.,0.,-210*cm" --gun.direction "0 0.05 0.99875" --gun.energy "10*GeV" --gun.particle="e-" -N 300 >& /data/users/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/output/sce_e_FSCEPonly_10.log

the geometry it is building is https://github.com/saraheno/DualTestBeam/blob/master/src/DRFtubeFiber_geo.cpp

the xml file that source uses is https://github.com/saraheno/DualTestBeam/blob/master/compact/SCEPCAL_DRFiber.xml

Info in <TGeoManager::CheckGeometry>: Fixing runtime shapes...
Info in <TGeoManager::CheckGeometry>: ...Nothing to fix
Info in <TGeoManager::CloseGeometry>: Counting nodes...
Info in <TGeoManager::Voxelize>: Voxelizing...
Compact          INFO  ++ Converted subdetector:EdgeDet of type DD4hep_EdgeDet [calorimeter]
Traceback (most recent call last):
  File "/data/users/eno/CalVision/dd4hep/DD4hep/bin/ddsim", line 25, in <module>
    RUNNER.run()
  File "/data/users/eno/CalVision/dd4hep/DD4hep/lib/python3.9/site-packages/DDSim/DD4hepSimula\
tion.py", line 294, in run
    kernel.loadGeometry(str("file:" + os.path.abspath(compactFile)))
cppyy.gbl.std.runtime_error: void dd4hep::sim::Geant4Kernel::loadGeometry(const string& compac\
t_file) =>
    runtime_error: std::bad_alloc
dd4hep: while parsing file:/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/DRF\
SCEPonly.xml
dd4hep: with plugin:DD4hep_XMLLoader

It is a pretty simple geometry, repeated a large number of times. any help would be appreciated

crash:

saraheno commented 9 months ago

Here is a longer version of the error dump

  placing fiber cell 200,199 at  (40.004,39.804)
giving it tower number 81203
    quartz fiber
  placing fiber cell 200,200 at  (40.004,40.004)
giving it tower number 81204
    scint fiber
exiting DRFtubeFiber creator
Compact          INFO  ++ Converted subdetector:DRFtubeFiber of type DD4hep_DRFtubeFiber [calorimeter]
DocumentHandler  INFO  +++ Loading document URI: /home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCAL\
_BOUND.xml [Resolved:'/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCAL_BOUND.xml']
DocumentHandler  INFO  +++ Document /home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCAL_BOUND.xml su\
ccesfully parsed with TinyXML .....
DocumentHandler  INFO  +++ Loading document URI: /home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCAL\
_EdgeDet.xml [Resolved:'/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCAL_EdgeDet.xml']
DocumentHandler  INFO  +++ Document /home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/SCEPCAL_EdgeDet.xml \
succesfully parsed with TinyXML .....
Creating EdgeDet
 det_id is 4
 det_name is .. EdgeDet
 half zlength 435.05 half height 288.367 half width 288.367 thickness 1
setting envelope visibility to EDGEVis1
setting EdgeDet side sensitive
setting EdgeDet side sensitive
setting EdgeDet side sensitive
exiting DRFiber creator
Info in <TGeoManager::CheckGeometry>: Fixing runtime shapes...
Info in <TGeoManager::CheckGeometry>: ...Nothing to fix
Info in <TGeoManager::CloseGeometry>: Counting nodes...
Info in <TGeoManager::Voxelize>: Voxelizing...
Compact          INFO  ++ Converted subdetector:EdgeDet of type DD4hep_EdgeDet [calorimeter]
Will Robinson
/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/DRFSCEPonly.xml
Traceback (most recent call last):
  File "/home/eno/CalVision/dd4hep/DD4hep/bin/ddsim", line 25, in <module>
    RUNNER.run()
  File "/home/eno/CalVision/dd4hep/DD4hep/lib/python3.9/site-packages/DDSim/DD4hepSimulation.py", line 296, in run
    kernel.loadGeometry(str("file:" + os.path.abspath(compactFile)))
cppyy.gbl.std.runtime_error: void dd4hep::sim::Geant4Kernel::loadGeometry(const string& compact_file) =>
    runtime_error: std::bad_alloc
dd4hep: while parsing file:/home/eno/CalVision/dd4hep/DD4hep/examples/DualTestBeam/compact/DRFSCEPonly.xml
dd4hep: with plugin:DD4hep_XMLLoader
saraheno commented 8 months ago

More information: I ran it with kernal verbosity = 1 (I think the crash is happening in a geant4 routine). The more verbose error message is given below.
Perhaps it is crashing in something called "DocumentHolder" which is a DDCORE routine? I do know it crashing in the DDCore routine DetectorImp.cpp at this line:

https://github.com/AIDASoft/DD4hep/blob/e812ea74eaaeae12e395ce29cc80b9eb0b6976b1/DDCore/src/DetectorImp.cpp#L849

Can somebody help me find the code corresponding to the Create call here, so I can try to find the DocumentHolder call?

*******************************************************************************

Compact          INFO  ++ Converted subdetector:EdgeDet of type DD4hep_EdgeDet \
[calorimeter]
DD4hep           DEBUG +++ Detector: Added detector EdgeDet to the world instan\
ce.
DocumentHolder   DEBUG +++ Release DOM document....
Info in <TGeoManager::CheckGeometry>: Fixing runtime shapes...
Info in <TGeoManager::CheckGeometry>: ...Nothing to fix
Info in <TGeoManager::CloseGeometry>: Counting nodes...
Info in <TGeoManager::Voxelize>: Voxelizing...
DocumentHolder   DEBUG +++ Release DOM document....
DocumentHolder   DEBUG +++ Release DOM document....
Traceback (most recent call last):
  File "/data/users/eno/CalVision/dd4hep/DD4hep/bin/ddsim", line 25, in <module\
>
    RUNNER.run()
  File "/data/users/eno/CalVision/dd4hep/DD4hep/lib/python3.9/site-packages/DDS\
im/DD4hepSimulation.py", line 307, in run
saraheno commented 8 months ago

More information: I have found the crash line. It is https://github.com/AIDASoft/DD4hep/blob/ec1b92a8a06b5759ee4eb891a83d418b6ed1f7d2/DDCore/src/DetectorImp.cpp#L747

so somehow some geometry being sent to GEANT4 is making it crash? But only when https://github.com/saraheno/DualTestBeam/blob/cfa3f0f88067b97e44b75933b5d4bd29485e9403/compact/SCEPCALConstants.xml#L23 is greater or equal to 200.

My code is very simple. It is just straws. I am a bit suprized it can cause this crash.

andresailer commented 8 months ago

Hi @saraheno

Can you specifically explain what your geometry is supposed to look like? It would be easier to see what can be changed if one knows what the geometry is supposed to be.

saraheno commented 8 months ago

It is like the IDEA fiber calorimeter. It is tubes of brass with either quartz or plastic fibers inside. It is a test beam array, so just a big rectangle.

saraheno commented 8 months ago

image

andresailer commented 8 months ago

The issue seems to be in ROOT::TGeoVoxelFinder::SortAll, but at least the crash might be fixed in ROOT 6.30.02 https://github.com/root-project/root/commit/653bcf4d4912eea9f22a064efd3aabe6a81a6d64

Could you pick up ROOT 6.30.02? E.g., via LCG_105?

Note that this fixes the crash in my local tests, I don't know if things are then behaving correctly! The function will end with

Error in <TGeoVoxelFinder::SortAll>: Data type (Int_t) overflow for volume DRFtubeFiber

This is triggered by a single volume having too many daughters, and then overflowing the integer data type to calculate the size for a new array to allocate here. Without the added check and abort the program ends with the bad_alloc exception you are observing.

PS:

I found the offending function by using gdb to catch throw std:bad_alloc.

gdb --args `which python3` `which ddsim` --compactFile=../compact/DRFSCEPonly.xml --runType=batch -G --steeringFile ../compact/SCEPCALsteering.py --outputFile=out_FSCEPonly_10GeV_e-.root --part.userParticleHandler='' -G --runType shell
> catch throw std:bad_alloc
> run
# wait for the exception
> bt
MarkusFrankATcernch commented 8 months ago

I am pretty much convinced that such a geometry can never work on machines you use here -- unless there is a severe bug there NOT really concerning the memory allocation. If the memory allocation already fails when closing the ROOT geometry, what do you expect happens when the corresponding Geant4 geometry is created on top ? This probably will require twice the memory....or even more since there is also physics.

If available, the code should once be run on a box with 1 TB memory to see if it is really the memory allocation. Then one has to find some other possibility how to construct such a geometry with less resources.

andresailer commented 8 months ago
UChar_t *ind = new UChar_t[nmaxslices*nperslice]

Was called with -217234848674 or something, because of overflow issues.

saraheno commented 8 months ago

Thanks Andre. I should be able to try the new root version later this week. (just to make sure: will this fix allow such a large calorimeter? or will it now just abort instead of crash? the latter wouldn't be useful :) )

should I have designed the geometry differently so a mother volume does not end up with so many daughters? Or is this just unavoidable for a design like the IDEA fiber that has odd high granularity?

andresailer commented 8 months ago

will this fix allow such a large calorimeter?

I don't know. It will give the error I mentioned above, but continue running the simulation, but I cannot validate the results.

should I have designed the geometry differently so a mother volume does not end up with so many daughters?

Probably it would be better to have a bit more hierarchy levels.

MarkusFrankATcernch commented 8 months ago

If you look in TGeoVoxelFinder::SortAll, the number of daughters enter in the new quadratic. Hence, any re-balancing of the geometry tree will greatly help to reduce the memory consumption. It will also help Geant4 to perform much better, because (as I was told) the G4 tracking depends strongly on the number of daughters in a given volume.

saraheno commented 8 months ago

okay, let me ask you a question then.

it was my impression that I should make a "calorimeter volume" and put my entire calorimeter into it. I do that at https://github.com/saraheno/DualTestBeam/blob/cfa3f0f88067b97e44b75933b5d4bd29485e9403/src/DRFtubeFiber_geo.cpp#L112

If I just put my fibers into the mother volume which I access at https://github.com/saraheno/DualTestBeam/blob/cfa3f0f88067b97e44b75933b5d4bd29485e9403/src/DRFtubeFiber_geo.cpp#L97C1-L97C1

will I still get the same crash beacuse I'm putting too many fibers into the mother volume?

And do I lose anything by not having a volume labled "calorimeter"? or could I just label https://github.com/saraheno/DualTestBeam/blob/cfa3f0f88067b97e44b75933b5d4bd29485e9403/src/DRFtubeFiber_geo.cpp#L122C6-L122C6 and such as calorimeter?

MarkusFrankATcernch commented 8 months ago
  1. it was my impression that I should make a "calorimeter volume" and put my entire calorimeter into it.

This generally is the correct approach.

  1. And do I lose anything by not having a volume labelled "calorimeter"? Labels should in general not matter unless they are empty strings. What matters is the path to a given volume ie. the concatenation of all physical volume labels. These are the PlacedVolume objects and their label is the label of the daughter volume + the copy number.

Generally "best" practice is an approach where all geometry leafs have a similar number of daughter volumes. This clearly is an ideal and never matches exactly what one wants to achieve. However, one can try to sort approach this goal. If I look at your pictures, instead of putting a large number of fibers into one single mother in 3 dimensions you could e.g. partition them

All these approaches would dramatically reduce the number of daughters by introducing additional levels of hierarchy and hence would reduce the memory allocation size. Clearly this is no excuse for the allocation failure due to integer overrun in ROOT, but it is a hint that this geometry was put together with brute force.

When building such sub-volumes as suggested above you should also already at this stage have an idea which sub-volumes require e.g. alignment with respect to neighbors, because it is advantageous that alignment procedures do not require afterwards to split such groups and you are able to align such groups with respect to each other....

saraheno commented 8 months ago

Thanks Markus. I think I understand. I think you are saying I can still put 400^2 fibers indirectly into the mother volume, but I should put them into layer column volumes and then put those column volumes into the calorimeter volume and put that into the mother volume.

saraheno commented 8 months ago

so now I'm remember why I switched to brute force geometry.

I just don't understand how to avoid duplicate volume labels.

if you look at my code at : https://github.com/saraheno/DualTestBeam/blob/0e1bca0c53dbe1639c7d80b8192e711245bb45a0/src/DRFtubeFiber_geo.cpp#L343

to me when this is added to the other volume labels at https://github.com/saraheno/DualTestBeam/blob/0e1bca0c53dbe1639c7d80b8192e711245bb45a0/src/DRFtubeFiber_geo.cpp#L315

there should be a unique element?

when I place the rows, do I have to somehow go into each row and and update each tower in each row with new volume labels?

MarkusFrankATcernch commented 8 months ago

I had a look at your code.... This is in principle a quite simple geometry. However, somehow it was diffucult for me to deduce a clear concept.... You must get a clearer idea how to model what should be constructed in order to arrive to something which at the end is usable and not only displays volumes. The code will then become much simpler and straight forward.

General remarks from what I understood from the code:

  1. A DetElement is the representation of a physical piece of the detector. Something "you can hold in your hand". DetElements are entities which really have to built physically when constructing the hardware. At the same time it should represent something sizable.
    • A "absoberhole" certainly does not fulfill this.
    • The tree of DetElements is a true tree, the geometrical tree is degenerate (daughters may be attached to multiple mothers).
    • Please read the first DD4hep publication to understand the difference between the structural hierarchy and the geometrical hierarchy. This difference must be clear. It must also be clear when to stop the hierarchy of DetElements.
  2. Once this is clear you have to build your subdetector. Make a sketch of what you want to build on a sheet of paper. First identify the hierarchy of your constituents, the so called structural element hierarchy (DetElement tree).
    • Each DetElement MUST correspond to the placement (PlacedVolume) of a logical volume.
    • The tree of volumes is the so called geometrical tree.
    • Logical volumes can be re-used and placed multiple times.
    • DetElements MAY NEVER be re-used - these must be either be created or deep-copied (cloned).

Now the enumeration of the sensitive elements (and ONLY sensitive elements should have a unique PhysVolID):

  1. The "system" tag should only go to the placement of the "envelopeVol"
  2. lines 255-300: I do not see a point why any single placement should have more than ONE physVoID. What is the point of having any additional identifiers. I cannot see any. 3) Only assign a PhysVolID to volumes which have at least one sensitive daughter in the hierarchy.

Otherwise:

  1. You should NOT declare DECLARE_DEPRECATED_DETELEMENT
  2. Your detector constructors should not start with "DD4hep_"
saraheno commented 8 months ago

Thanks Markus. On https://dd4hep.web.cern.ch/dd4hep/page/publications/, which is the publication to which you refer? I did look at: https://dd4hep.web.cern.ch/dd4hep/usermanuals/DD4hepManual/DD4hepManualch1.html#x2-10001 but really did not find the prose there anywhere near as helpful as what you just wrote here.

I do know that I really don't understand the conceptual framework well. What should be a DetElement (and how do I do the "hole" if it is not a DetElement?) The examples in the examples area of dd4hep have virtually zero comments to help one understand...

if you could point me to something in nice prose like you just wrote here explaining the philosophy, it would be very helpful.'

(lines 255-300 were my attempt to evade the the problems I was having with geant complaining that I had assigned identical volume ids and exiting... and since I am doing calorimeter studies, my entire detector is sensitive. I want to see what is going on in the absorber as well as in the fibers that produce the signal. Not only am I designing a detector for FCC, I am also trying to understand what is limiting the calorimeter and for that I need to know exactly where all the energies are going)

And my hierarchy: there are brass straws containing scintillating or quartz fibers which have photodetectors at the ends. That is it. There is no hierarchy beyond that. It is a very simple detector, so clearly it should be simple to implement. That is what I try to do. I have a calorimeter volume. It contains these straws. That was causing the root crash, so I tried to first put the straws into rows and put the rows in the calorimeter. But now I have duplicate volume ids because I don't seem to understand how to assign them when I have something in something in something. I can only get them right when I just put things directly into the calorimeter volume.

Maybe what I need is an example that shows how to build up a detector element that contains many sensitive volumes and assigns a unique id to each one? Is there one like that in the examples area?

MarkusFrankATcernch commented 8 months ago

This is the picture explaining the difference between the structural hierarchy and the geometrical hierarchy: https://dd4hep.web.cern.ch/dd4hep/usermanuals/DD4hepManual/DD4hepManual4x.png in section 1.3.1 of https://github.com/AIDASoft/DD4hep/issues/1173 This is the key idea of dd4hep with respect to geometry only descriptions.

saraheno commented 8 months ago

Thanks Markus. A nice picture, but it still doesn't show me how to structure my code.

Is there an example you can point me to that is fairly simple where a DetElement has several volumes of different shapes inside of it that are active?

saraheno commented 8 months ago

Hi Markus Looking more carefully at DD4hepManual4x

And trying to apply it to my case.

On the left you have the DetElements.

For me, TPC translates to Calorimeter

EndcapA and B might translate to say the 200 rows

Sectors might translate to the 200 towers in each row

And then there would be an additional layer on my equivalent of your left here where each tower contains a brass tube, the fiber in the tube, the air in the gap between the fiber and the brass, and then the sipms at the ends of the tower.

On the right, you have the Volumes and the PlacedVolumes and there is kind of a one-to-one correspondence between the left and right objects.

Now let me ask you a question about the difference between EndCapA/EndcapB (DetElements) and TPCEndcap (a volume). TPCEndcap is the more abstract? And EndCapA/EndcapB are specific instances of that more general class?

Then I don't understand this. If TPCEndcap is abstract, and EndCapA/EndcapB are specific instances, why does TPCEndcap need a Placement? I guess it is placed in TPC. But why are there 2 placements? One is for EndcapA and one if for EndcapB? Why doesn’t the placement only concern the actual detector instead of its abstraction?

Also I know it is the Placedvolumes that get the volume labels which are causing me problems. My practical question is this. If place a TPCSector into a TPCEndcap and then make two TPCEndcaps, EndCapA and EndcapB, how do I make the sure volumelabels for EndcapA and EndcapB are unique? especially it seems I've made TPCSectors and put them in TPCEndCap. Then I close TPCEndcaps to get EndCapA and EndCapB. But then how do I get the all the sectors on the right to have unique labels?
Do I have to plow down through down into TPCEndcap each time I connect it to a detector element and put new volumes on? It is the plowing down I don’t quite understand how to code… It seems to me that when I place TPCEndcap to EndcapA, the placements of TPCSector into TPCEndcap don't get their labels updated to be unique. But if that doesn't happen, what is the point of making TPCEndCap and loading TPCSectors into it? It is like I have to do the brute force twice. Why not just make EndCapA and EndCapB directly?

saraheno commented 8 months ago

or maybe let me ask it this way, referring again to

DD4hepManual4x

Supposed I make DetElement ENDCcapA and put in it sectors A-N. Then I cone it to make EndCapB which contains sectors M-Z. I also make Volumes TPCEndcap and put in it the TPC sector.

How do I do the lines connecting the things in code? especially how do I do create the placements between TPCEndcap and TPCSector? (and why do both sector A and Z go to the same placement? won't that create duplicate volume labels?) And what are those placements hanging off the bottom of TPCSector?

saraheno commented 8 months ago

also I've cleaned up the code a bit. removed the bad attempt to get rid of the duplicate labels.

but I still clearly don't understand how to avoid them. To me, the code looks like every element has its own label. There is ix,iy to distinguish rows and columns. And there is type to say what part of the tower (brass, fiber, phototube, air) it is. To me these are unique. But I still get duplicate volumes, so I obviously don't know how to do those lines properly in your diagram.

and the lines I think do the deep clones are

https://github.com/saraheno/DualTestBeam/blob/289df2d1c30e04d41836ac688ea072cb78ab5362/src/DRFtubeFiber_geo.cpp#L304

and

https://github.com/saraheno/DualTestBeam/blob/289df2d1c30e04d41836ac688ea072cb78ab5362/src/DRFtubeFiber_geo.cpp#L331

MarkusFrankATcernch commented 8 months ago

There is actually an implementation of the TPC shown in the picture in the examples: examples/AlignDet/src/AlpehTPC_geo.cpp and correspondingly: examples/AlignDet/compact/AlephTPC.xml. The example shows how exactly the structural and the geometrical trees of this picture are built and connected to each other.

What the example does not show are the sensitive areas (wire-planes) in front of the sectors. These however would not be added as individual DetElements (a single wire you do not really anymore "hold in your hand") and hence would not enter the structural tree, but only the geometrical tree to allow Geant4 do its job). Same holds for individual pad-rows to collect the mirror charge of the wires. You can get inspired by this example though the sector geometries are more complicated than in you calorimeter.

MarkusFrankATcernch commented 8 months ago

If you really want to go through a code cleanup, we can do it together. It will you what to remove and you then check if the geometry is still the same.....I will not do it for you, since next time you have to be able to do it yourself, but I offer to assist and comment.

One question upfront: To me the calorimeter in the picture above is a so called spaghetti calorimeter (spaghetti=tube). How do the spaghettis relate to towers? Normally towers are bound to layered calorimeters with some padded projective readout segmentation internally.

Now the proposed changes:

1) If I see things correct: Everything which is abs1/2_det, absh1/2_det, fiber1/2_det and photod1/2_det is not really useful. They are all end-points of the structural tree and do not need to be DetElements, since they

So far the geometry should not at all have changed, but the code is likely 30% smaller. Then we will go to the next step.

P.S. please also remove multiple empty lines. Makes things simple worse readable. See you then for the next iteration.

saraheno commented 8 months ago

Markus, I'd love to have a zoom meeting with you to understand this better. I am working with a large number of students and would really like to help them better. Today I'm chairing the DPF24 planning committe (and seeing the eye doctor). I'm in the Eastern US so it would have to be after 14:00 your time. What day of the week would be good? Because there are many things I don't understand how to code in your email (just being an old fortran programmer) like how to not have abs1/2_det, absh1/2_det, fiber1/2_det and photod1/2_det as detector elements and yet still read the energies and numbers of photons in them separately. To me they need a volume label, and volume labels are attached to placedvolumes which are attached to volumes which are attached to detector elements. so there is something I'm not understanding.

Due to the eye dialation, I cannot clear the code more today but will start either tonight or tomorrow. Once the semester begins the week of 29 Jan (plus FCC ped week in annecy), my coding time drops to zero.

MarkusFrankATcernch commented 8 months ago

Online cluster is down tomorrow and Wednesday. I shall then be in the office. Could do then... Simply send me a mail upfront or phone me on 16-2100 - if I do not forget the phone at home like today :-( Should not be too difficult to translate my github name to email with the usual conventions.

saraheno commented 8 months ago

also, in regard to your upfront question...

To me the calorimeter in the picture above is a so called spaghetti calorimeter (spaghetti=tube). How do the spaghettis relate to towers? Normally towers are bound to layered calorimeters with some padded projective readout segmentation internally.

"tower" is just my word for tube. To me, a tower is something that points towards the interaction point in a real detector, something whose long direction is parallel to the particle direction. In my toy test beam module, it is something that is parallel to the test beam particle direction. This test beam module is just a bunch of tubes with their long axis parallel to the test beam direction.

This calorimeter has no segmentation perpendicular to the beam direction (well, it sort of does... if you think of the sipmm-fiber-sipmm as "layers". one sipmm is the first thing the beam encouters, then the fiber or the absorber, depending on where in x-y it hits (z in parallel to the beam), then the last sipmm

saraheno commented 8 months ago

Thanks for the help, Markus. The new code is fast and short.

I wanted to now add back in the photodetectors at each end of each fiber.

If I look at your scheme https://github.com/saraheno/DualTestBeam/blob/e73c86ce618787aec484792ead318541f326e811/src/DRFtubeFiber_geo.cpp#L19

// Detector geometry structure // // /world_volume/FiberCalo/rowtube_1/brass_1/hole_1/quartz_1 // /brass_2/hole_1/scintillator_1 brass_1 == brass_2 == brass....n // /brass_3/hole_1/quartz_1
// /brass_4/hole_1/scintillator_1 // /brass_5/hole_1/quartz_1 // brass_1/quartz_1 Volume(brass) / hole // Volume(hole) / quartz // alt: Volume(hole) / scintillator

I am trying to figure out where to put it. To me, it should go with the "brass"? so

/world_volume/FiberCalo/rowtube_1/brass_1/hole_1/quartz_1
/brass_1/photodet1_1
/brass_1/photodet2_1
/brass_2/hole_1/scintillator_1
/brass_2/photodet1_1 /brass_2/photodet2_1

Or do I need to make a rowphotodet so

/world_volume/FiberCalo/rowtube_1/brass_1/hole_1/quartz_1 /brass_2/hole_1/scintillator_1
/brass_N/hole_1/X_1 /rowphoto_1/phdet1_1 /phdet1_2 /phdet1_N /rowphoto_2/phdet2_1 /phdet2_2 /phdet2_N

/world_volume/FiberCalo/rowtube_1/brass_1/hole_1/quartz_1 /brass_2/hole_1/scintillator_1

saraheno commented 8 months ago

The first choice seems to work so I'm closing this. thanks again!