| Summary: | Unknown units are accepted and treated as 0, should be fatal. Incorrect units are also accepted, should be fatal. | ||
|---|---|---|---|
| Product: | Geant4 | Reporter: | gahs |
| Component: | persistency/gdml | Assignee: | Witold.Pokorski |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | davidl |
| Priority: | P5 | ||
| Version: | 10.2 | ||
| Hardware: | All | ||
| OS: | All | ||
Thanks a lot for spotting this and for suggesting the solution. I went through all the files and I have added those checks. Cheers, Witek This fix seems to have broken my simulation. My GDML file is produced by ROOT and contains several sections like the following:
<divisionvol axis="kPhi" number="48" width="7.5" offset="-3.75" unit="deg">
<volumeref ref="sd07"/>
</divisionvol>
I believe the issue is that the unit is "deg" and so the check on line 160 of G4GDMLReadStructure.cc that the units are of type "Length" fails. This throws an exception causing the parsing to fail altogether.
If this is the case, could you put in cases for any other valid units when making this type of check?
Thanks for reporting this. Indeed the check was not correct for angular units. I have modified the code to check along which axis the division happens. The code is in SVN and tagged gdml-V10-02-07. I hope this will solve your problem. Cheers, Witek |
Incorrect units are accepted by the parser resulting in wrong values, and unrecognized units are accepted resulting in a zero value. In the first case, no warning is given; in the second case execution proceeds with just a warning. The latter problem was noticed by LArSoft developers for materials with densities specified in "g/cc", but the issue is more general. The problem arises only in version 10.1.p2 and later. Starting with 10.1.p2 the conversion of units in G4GDMLReadMaterials is done using G4UnitDefinition::GetValueOf(). If a unit is not recognized, G4UnitDefinition::GetValueOf() returns 0 and prints only a warning message. Execution then continues with a 0 value for the quantity. This is highly undesirable, since 0 is almost guaranteed to be a nonsense value. In addition, it is undesirable that nonsense units for a quantity be accepted, e.g., "pascal" for a density. Using G4UnitDefinition::GetCategory() you can fix both problems. I would suggest the following bug fix for both of the above problems: use G4UnitDefinition::GetCategory() to check the unit type in the GDML parsing routines. If the return value does not match the expected unit category, throw a fatal error. Since GetCategory() returns "None" for an unrecognized unit, this will also take care of the unrecognized unit problem. E.g., in G4GDMLReadMaterials::DRead() [G4GDMLReadMaterials.cc:154], change if (attName=="unit") { unit = G4UnitDefinition::GetValueOf(attValue); } to if (attName=="unit") { unit = G4UnitDefinition::GetValueOf(attValue); if (G4UnitDefinition::GetCategoryOf(attValue)!="Volumic Mass") { G4Exception("G4GDMLReadMaterials::DRead()", "InvalidRead", FatalException, "Invalid unit for density!"); } } and similarly in AtomRead(), TRead(), PRead(), and MEERead(), where the unit categories should be "Molar Mass", "Temperature", "Pressure", and "Energy", respectively. Similar checks are also needed in some other GDML parsing routines that have switched from using Evaluator to G4UnitDefinition for the units parsing. In versions of Geant4 prior to 10.2.p2 the unit parsing was done using CLHEP's Evaluator, which dealt with undefined symbols in its own way. I'm glad the switch was made to parsing units using G4UnitDefinition because there are reasons to prefer G4UnitDefinition over Evaluator, such as the separation of the unit namespace from variables, and especially the possibility unit category checks. But protection against unrecognized units is sorely needed.