Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Name duplicates of materials not always checked #195

Open
ManuelHu opened this issue Oct 24, 2024 · 4 comments
Open

Name duplicates of materials not always checked #195

ManuelHu opened this issue Oct 24, 2024 · 4 comments

Comments

@ManuelHu
Copy link
Contributor

ManuelHu commented Oct 24, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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:

  1. It is possible to produce invalid GDML files using this loophole, as pyg4ometry.gdml.Writer happily writes out such structures.
  2. [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]
  3. but there are other cases, where actually invalid GDML could be produced, silently...

@stewartboogert
Copy link
Member

stewartboogert commented Oct 25, 2024

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
Copy link
Contributor Author

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants