Name duplicates of materials not always checked

ManuelHu commented 4 days ago

Two small test cases, the first checks the name duplicate; the second one does not. The only difference is that no registry is passed in the second element constructor.

from pyg4ometry import geant4 as g4

reg = g4.Registry()

try:
    el=g4.ElementSimple(name="X", symbol="X", Z=1, A=1, registry=reg)
    x = g4.Material(name="X", density=1, number_of_components=1, registry=reg)
    x.add_element_natoms(el, natoms=1)
except Exception as e:
    print("SAME REGISTRY", e)

reg = g4.Registry()

try:
    el=g4.ElementSimple(name="X", symbol="X", Z=1, A=1)  #, registry=reg)
    x = g4.Material(name="X", density=1, number_of_components=1, registry=reg)
    x.add_element_natoms(el, natoms=1)
except Exception as e:
    print("DIFFERENT REGISTRY", e)

output:

SAME REGISTRY Identical material name detected in registry: X

Interestingly, the second still produces a GMDL file that Geant4 can read just fine - i.e. elements and materials appear to have different "namespaces"/stores in Geant4?

Maybe pyg4ometry should also split the names to check? (I know, that would be quite a substantial checks)

stewartboogert commented 3 days ago

So in general this is the expected behaviour. Not adding the registry to the constructor means there isn't actually a name clash.

The issue of this is fundamentally GDML is a global name space file format. The registry exists to check name clashes in a name space. So a registry is a namespace. Not giving the registry is the way of not adding a "name" i.e X to the material name space.

So there isn't an easy way to check for name clashes without explicity providing the registry.

ManuelHu commented 3 days ago

This is essentially a bug I found in our (LEGEND) code, that was just masked for a very long time. The bug in our code is now fixed...

So this issue just boils down to some things I noticed; but I am not sure if we should/can fix them:

It is possible to produce invalid GDML files using this loophole, as pyg4ometry.gdml.Writer happily writes out such structures.
[in this case, we never saw the problem, as Geant4 seems to not only have a single material namespace, but it is split into seperate namespaces for elements and materials. pyg4ometry is (in this case) stricter then Geant4 requires]
but there are other cases, where actually invalid GDML could be produced, silently...

stewartboogert commented 3 days ago

So if I understand correctly. This only punches through with elements/materials? I suppose it could be solids too. I think maybe the best way to proceed is to warn the user the GDML is not valid. I'll take a look and get back to the issue.

So the convention around adding or not adding the registry is related to if something should be written out at the top of the gdml. This is particularly relevant for defines, positions etc. Not relevant at all for materials. So it reduces to the case if we want to define something temporarily and not write it. This is definitely the case for variables and potentially solids. So two solutions are

1) Enforce elements, isotopes and materials have to have a registry 2) Check the material -> element -> isotope chain is valid and warn the user.

What you you think?

ManuelHu commented 2 days ago

Yeah, it could also be the case or solids (i.e. regstering a LV with an unregistered solid).

One idea I had (somewhat similar to your idea "2"):

the writer stores dictionaries of what it had written (name -> object)
warn the user if a name is duplicated and has a different instance. This should not impact normal usage, if everything is connected to the registry.
At the moment, the writer already has similar things to avoid writing the same element twice. But it only stores the names of written elements, so it cannot be used to warn the user.

In the case of materials the current check is just "too late" to check the recursive relation Material -> Element -> Isotope:

https://github.com/g4edge/pyg4ometry/blob/main/src/pyg4ometry/gdml/Writer.py#L267-L268 checks if the name already exists
https://github.com/g4edge/pyg4ometry/blob/main/src/pyg4ometry/gdml/Writer.py#L357-L358 adds the name to the materials_written list
and in between the recursive writeMaterial calls are made, not catching the duplicate name in Material->Element->Isotope chains

g4edge / pyg4ometry

Name duplicates of materials not always checked #195