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

minor fix in gdml code #19

Closed
wants to merge 1 commit into from
Closed

Conversation

shangjiaxuan
Copy link

Auxilary info may contain other info, returning at early stage when loading regions seem inappropriate.

The two lists are used to write the region exports in the process of serialization, and does not seem to be influenced by whatever serializer it's using.

Auxilary info may contain other info, returning at early stage when loading regions seem inappropriate.

The two lists are used to write the region exports in the process of serialization, and does not seem to be influenced by whatever serializer it's using.
@gcosmo
Copy link
Contributor

gcosmo commented Feb 2, 2021

The requested modification refers to method ImportRegions() in G4GDMLParser and is not correct. Regions are treated as auxiliary information in Geant4 and that method is supposed to treat only the presence of Region tagged auxiliary types. Generic auxiliary types must be treated directly in the user code.
Example G01 in the examples/extended/persistency/gdml module shows how to import generic auxiliary information content.

@gcosmo gcosmo closed this Feb 2, 2021
@shangjiaxuan
Copy link
Author

shangjiaxuan commented Feb 2, 2021

The requested modification refers to method ImportRegions() in G4GDMLParser and is not correct. Regions are treated as auxiliary information in Geant4 and that method is supposed to treat only the presence of Region tagged auxiliary types. Generic auxiliary types must be treated directly in the user code.
Example G01 in the examples/extended/persistency/gdml module shows how to import generic auxiliary information content.

Hi @gcosmo, the problem is not that I want it to treat other types, but that if some other type occurs first, and then the region part will get ignored. This, I do not think is expected behavior.

The change is to change the behavior to silently ignore the xml element if it's not Region instead of stop the processing.

Fixing this will also make inheriting G4GDMLParser and implementing from auxmap and auxlist much easier (Writer inherit from WriteStructure and overload the tree accessing function to add the auxiliary info, then call the base function, which will write the info to file. Reader can read this info anyway from auxmap.).

Also, what about the rlist and ullist lifetime management?

@gcosmo
Copy link
Contributor

gcosmo commented Feb 2, 2021

Can you please provide a test case for which a problem occurs? G4GDMLParser is not meant to be inherited and ImportRegions() is only meant to import just geometrical regions.

@shangjiaxuan
Copy link
Author

shangjiaxuan commented Feb 2, 2021

Hi @gcosmo I just added a few lines of code to the stock auxiliary.gdml just before the <setup /> section as follows (This will make the parser abort, thus the region is not loaded):

<userinfo>
 <auxiliary auxtype="myaux" auxvalue="myvalue"/>
 <auxiliary auxtype="Region" auxvalue="region_name">
  <auxiliary auxtype="volume" auxvalue="Boxvol"/>
 </auxiliary>
</userinfo>

Whereas switching the order will work properly (The region is loaded, and myaux is ignored):

<userinfo>
 <auxiliary auxtype="Region" auxvalue="region_name">
  <auxiliary auxtype="volume" auxvalue="Boxvol"/>
 </auxiliary>
 <auxiliary auxtype="myaux" auxvalue="myvalue"/>
</userinfo>

@gcosmo
Copy link
Contributor

gcosmo commented Feb 2, 2021

Have you tried properly closing the custom auxiliary block?
i.e.:

 <auxiliary auxtype="myaux" auxvalue="myvalue">
</auxiliary>

@shangjiaxuan
Copy link
Author

shangjiaxuan commented Feb 2, 2021

Have you tried properly closing the custom auxiliary block?
i.e.:

 <auxiliary auxtype="myaux" auxvalue="myvalue">
</auxiliary>

That will make the XML invalid with exception thrown at GDMLReadSetup::getSetup() in the problematic case because the setup will be after the XML become invalid (and stop parse). myaux is already complete in one line with /> on the same line.

Edit: Just noticed your code did not have the / in the line. In this case fRegion is still NULL after reading in the first case, an initialized region in the second.

@gcosmo
Copy link
Contributor

gcosmo commented Feb 2, 2021

Sorry, I don't understand what you mean.
You need to close the auxiliary block at the top level.
If you add the proper terminated block to the stock auxiliary.gdml either before or after the region block, you get all information properly parsed:
`Auxiliary Information is found for Logical Volume : Boxvol
|SensDet : veloSD
|sometype : somevalue
||somesubtype : somesubvalue

Global auxiliary info:

|myaux : myvalue
|region : myregion1
||RootLogicalVolume : Boxvol1
|||sometype : somevalue
||RootLogicalVolume : Boxvol2
||proton_cut_legth : 2 mm
||proton_cut_energy : 900 KeV
||electron_cut_length : 1 mm
||electron_cut_energy : 1 MeV
||cut : electron
|||energy : 900 keV
||userlimit_electron : 1 keV
|region : myregion2
||RootLogicalVolume : vol1
||RootLogicalVolume : vol2
||electron_cut_length : 1.5 mm
||proton_cut_length : 2.5 mm
`

@shangjiaxuan
Copy link
Author

Sorry, I don't understand what you mean.
You need to close the auxiliary block at the top level.
If you add the proper terminated block to the stock auxiliary.gdml either before or after the region block, you get all information properly parsed:
`Auxiliary Information is found for Logical Volume : Boxvol
|SensDet : veloSD
|sometype : somevalue
||somesubtype : somesubvalue

Global auxiliary info:

|myaux : myvalue
|region : myregion1
||RootLogicalVolume : Boxvol1
|||sometype : somevalue
||RootLogicalVolume : Boxvol2
||proton_cut_legth : 2 mm
||proton_cut_energy : 900 KeV
||electron_cut_length : 1 mm
||electron_cut_energy : 1 MeV
||cut : electron
|||energy : 900 keV
||userlimit_electron : 1 keV
|region : myregion2
||RootLogicalVolume : vol1
||RootLogicalVolume : vol2
||electron_cut_length : 1.5 mm
||proton_cut_length : 2.5 mm
`

The information will always be parsed and stored in the list, but the logic in ImportRegions will stop when it encounters the first aux that is not Region (since the reader uses push_back, it's the same order as the xml). This means the G4VPhysicalVolume* world tree got from the parser will not include the regions defined. A valid gdml file with extra field will lose some of the base data (region info may or may not be imported to world).

@gcosmo
Copy link
Contributor

gcosmo commented Feb 3, 2021

@shangjiaxuan, I have analysed the auxiliary.gdml sample provided in the example and found it is not up to date, therefore not exposing the problem you're highlighting here. After having corrected the sample, I can reproduce the problem, and indeed your proposed corrections are fine. These will be included in the next patch-release. Thanks!

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