-
Notifications
You must be signed in to change notification settings - Fork 1
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
Full conversion #2
base: main
Are you sure you want to change the base?
Conversation
Added in section to convert the entries be converted in to Reg-expressions
… process of converting can be done in one step (this is the eventual goal)
…r so that it does it all in one go. Config file is still required
bin/full_conversion.py
Outdated
required=True, type=str, nargs='+') | ||
parser.add_argument('-o', '--out', help='Converted file path', | ||
default='nothing', type=str) | ||
parser.add_argument('-v', '--visibility', help='Level of layers in detector', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parser.add_argument('-v', '--visibility', help='Level of layers in detector', | |
parser.add_argument('-v', '--visibility-level', help='Level of layers in detector to consider in conversion', |
Just to make it easier to understand
bin/full_conversion.py
Outdated
parser.add_argument('-o', '--out', help='Converted file path', | ||
default='nothing', type=str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parser.add_argument('-o', '--out', help='Converted file path', | |
default='nothing', type=str) | |
parser.add_argument('-o', '--out', help='Converted file path', | |
default='', type=str) |
Unlikely in this case but people might want to use whatever magical string that you put here. You can then no longer differentiate between that magic string and an actual user value. It's easier to deal with an empty string here, because an empty output file doesn't make sense.
bin/full_conversion.py
Outdated
|
||
args = parser.parse_args() | ||
|
||
convert(args.compact, args.out, args.visibility) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert(args.compact, args.out, args.visibility) | |
convert(args.compact, args.out, args.visibility_level) |
To keep things working after the argparse changes
bin/full_conversion.py
Outdated
ROOT.gGeoManager.SetVisLevel(visibility) | ||
ROOT.gGeoManager.SetVisOption(0) | ||
|
||
if out_path == 'nothing': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if out_path == 'nothing': | |
if not out_path: |
to keep things working after the argparse changes
bin/full_conversion.py
Outdated
counter = 0 | ||
slash_list = [] | ||
for cfile in compact_files: | ||
for i in cfile: | ||
counter += 1 | ||
if i == '/': | ||
slash_list.append(counter) | ||
|
||
last_slash = max(slash_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this into a function to declutter the conversion function. I am also not entirely sure I understand what the output will be yet if there are multiple compact files.
The end result would look something like
# ... all the rest from above
ROOT.gGeoManager.SetVisOption(0)
out_path = determine_outpath(out_path, compact_files) # to be implemented
ROOT.gGeoManager.Export(out_path)
bin/full_conversion.py
Outdated
convert(args.compact, args.out, args.visibility) | ||
|
||
|
||
def convert(compact_files, out_path, visibility): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be the same function as in conversion_xml2root.py
so we could also just import it from there instead of copying it over.
…file so that an auto-generated configuration file can be created.
…w a certain amount of layers in your detector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. I have left a few comments on the implementation of tree
that should make it independent of any global state / variables and some explanation on why that is a good thing to aim for.
One general thing for the configfile_generator.py
script is the following. I would put everything that is not yet inside of a function into a main
function (similar to what you already do for the full_conversion.py
). That way we can then simply do (with many gaps for you to fill ;) )
from configfile_generator import tree, post_process`
# ... the rest of the code that loads the geometry from DD4hep ...
if not args.config_file: # or whatever the exact argument name is
det_tree = tree(description)
config = post_process(det_tree) # potentially with more arguments
# write config file
# ... The rest of the code that does the root -> gltf conversion (calls node) ...
configfile_generator.py
Outdated
# start = theDetector.detector("OpenDataTracker") | ||
start = theDetector.world() | ||
|
||
def tree(detElement, depth): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def tree(detElement, depth): | |
def tree(detElement, maxDepth=5, depth=0): |
Passing in maxDepth
as argument makes it possible to have this function being fully self contained and not depend on any other global / external values (like args.maxDepth
at the moment). This makes it possible to import this function from another python module without any weird surprises because the value of that global variable might depend on whatever path is taken during imports.
Giving depth
a default value of 0
makes it possible to simply call this function without it. Without this default value all users that call this will have to know that they need to pass in 0
here in order to get to their desired output.
configfile_generator.py
Outdated
depth += 1 | ||
children = detElement.children() | ||
for raw_name, child in children: | ||
if depth > args.maxDepth: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if depth > args.maxDepth: | |
if depth > maxDepth: |
To use the argument that we pass rather than some global variable.
configfile_generator.py
Outdated
depth += 1 | ||
children = detElement.children() | ||
for raw_name, child in children: | ||
if depth > args.maxDepth: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if depth > args.maxDepth: | |
if depth > maxDepth: |
To use the argument that we pass rather than some global variable.
configfile_generator.py
Outdated
if depth > args.maxDepth: | ||
tree(child, depth) | ||
else: | ||
dictionary = tree(child, depth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dictionary = tree(child, depth) | |
dictionary = tree(child, maxDepth, depth) |
Now we also need to pass this in the recursion, obviously.
configfile_generator.py
Outdated
print('yay') | ||
|
||
|
||
detector_dict = tree(start, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detector_dict = tree(start, 0) | |
detector_dict = tree(start, maxDepth=args.maxDepth) |
And this becomes the place where we "connect" the value that we got from parsing the command line arguments with the value we use in the function.
…to the orrect form for the json file and created an output for automatic json file to be created so that this can be added into the complete .xml to .gltf conversion as an automatic config file. Also made some edits to silly mistakes in the 'tree' function.
… to the full conversion of xml to gltf. Have started the full conversion included the automatic config file however dd4hep and ROOT python imports seem unable to work together in the same python script so I am currently thinking of using python subprocess to still run the conversion together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good already. We will have to do some reorganizing and refactoring to remove some of the duplicated code, but we will do that once I am back. I would move some of the files around:
- All the python scripts should go into
bin
- If you have auto-generated json config files, I would put them into a new
configs/examples
folder, where you can also put aREADME.md
file that can hold 1-2 sentences on how they were obtained.
configfile_generator.py
Outdated
def post_processing(obj, main_parts, subParts={}, sublist= []): | ||
for k, v in obj.items(): | ||
if k in main_parts: | ||
sublist = [f'{k}_(?!envelope)\\w+'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the people who don't know regex by heart (like me). Could you put a comment here briefly explaining what (!?envelope)
does in the generated regular expression?
config_automatic.json
Outdated
@@ -0,0 +1 @@ | |||
{"childrenToHide": [], "subParts": {"BeamCal": [["BeamCal_(?!envelope)\\w+"], 0.8], "Coil": [["Coil_(?!envelope)\\w+"], 0.8], "EcalBarrel": [["EcalBarrel_(?!envelope)\\w+"], 0.8], "EcalEndcap": [["EcalEndcap_(?!envelope)\\w+"], 0.8], "EcalPlug": [["EcalPlug_(?!envelope)\\w+"], 0.8], "FTD": [["FTD_(?!envelope)\\w+", "FTDDisk_.*_negZ", "ftd_petal_negZ_.*_.*", "ftd_sensor_negZ_.*_.*_.*", "FTDDisk_.*_posZ", "ftd_petal_posZ_.*_.*", "ftd_sensor_posZ_.*_.*_.*", "FTD_support"], 0.8], "HcalBarrel": [["HcalBarrel_(?!envelope)\\w+"], 0.8], "HcalEndcap": [["HcalEndcap_(?!envelope)\\w+"], 0.8], "HcalRing": [["HcalRing_(?!envelope)\\w+"], 0.8], "LHCal": [["LHCal_(?!envelope)\\w+"], 0.8], "Lcal": [["Lcal_(?!envelope)\\w+"], 0.8], "QD0_cryostat": [["QD0_cryostat_(?!envelope)\\w+"], 0.8], "QD0_support": [["QD0_support_(?!envelope)\\w+"], 0.8], "SET": [["SET_(?!envelope)\\w+", "set_ladder_.*_.*", "set_ladder_.*_.*_.*"], 0.8], "SIT": [["SIT_(?!envelope)\\w+", "sit_ladder_.*_.*", "sit_ladder_.*_.*_.*"], 0.8], "SServices00": [["SServices00_(?!envelope)\\w+"], 0.8], "TPC": [["TPC_(?!envelope)\\w+", "tpc_endcap_bwd", "tpc_endcap_fwd", "tpc_sensGas_bwd", "tpc_row_bwd_.*", "tpc_sensGas_fwd", "tpc_row_fwd_.*"], 0.8], "Tube": [["Tube_(?!envelope)\\w+"], 0.8], "VXD": [["VXD_(?!envelope)\\w+", "BerylliumAnnulusBlock_.*_negZ_.*", "BerylliumAnnulusBlock_.*_posZ_.*", "ElectronicsEnd_.*_negZ_.*", "ElectronicsEnd_.*_posZ_.*", "SiActiveLayer_.*_negZ_.*", "SiActiveLayer_.*_posZ_.*", "VXD_support", "KaptonLines_.*_negZ", "KaptonLines_.*_posZ", "MetalLines_.*_negZ", "MetalLines_.*_posZ", "VXD_ExtKaptonCab_neg", "VXD_ExtKaptonCab_pos", "VXD_ExtMetalCab_neg", "VXD_ExtMetalCab_pos", "VXD_endplate_bwd", "VXD_endplate_fwd", "VXD_support_bwd_inner", "VXD_support_bwd_outer", "VXD_support_fwd_inner", "VXD_support_fwd_outer", "negShellCone", "posShellCone"], 0.8], "YokeBarrel": [["YokeBarrel_(?!envelope)\\w+"], 0.8], "YokeEndcap": [["YokeEndcap_(?!envelope)\\w+"], 0.8]}, "maxLevel": 3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an auto-generated output? Very nice. If you want to make it more readable you can use the indent
parameter of the json.dump
function.
configfile_generator.py
Outdated
detector_dict = tree(start, 0, args.maxDepth) | ||
#pprint.pprint(detector_dict) | ||
|
||
def post_processing(obj, main_parts, subParts={}, sublist= []): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also be made to take a list of subParts to ignore? That would make it possible to define a list that can be put into childrenToHide
without leading to conflicts
…e geometries and provide an automatic configuration file. I have made a new function 'determine_outpath' to give an automatic path for the files if they are not given.
… to be added to. Tidied up the code a bit to make it more usable and readable
… running of the script so that they can delete the root file that has been created once they have the gltf file and they can also stop the python script early to modify the config_file. I then also responded to some of the comments so everything has been checked and cleaned up a bit.
@tmadlener Hi to respond to a few of your comments: I have changed most of the suggestions you have made. I realise I was late to putting in a few of them so sorry about that :) . The suggestions I haven't added in yet includes adding the childrenToHide subparts. Should I have this as something the user can input and say for example I don't want to see this part of the detector? And also I wasn't able to import the configfile_generator.py to call on it with the conversion script as there seems to be a clash with dd4hep and ROOT in the C++ files. So I'm using subprocess which is working well. If you have anymore comments on what I've written let me know :) |
…). Also trying to add in a way to add hidden-children into the automatic config file creation and maybe find a way for multiple files to be inputed at one time.
…r that they want to have removed to 'childrenToHide' so that they are not displayed on the gltf file
…ng instructions in the README.md of how the python scripts can be used and what parameters they take in
…reg ex wouldn't check for capital letters as some parts of the detector have stopped getting called due to a change of capitalisation.
…nd to set the ILD default colours for the Phoenix event display. Not finished and very messy but I've set what each subpart of the detector colour should be I just need to add in the colours and then see if it works. This is all done in phoenixExport.js
… colours they want for each subdetector into the config file and it will apply it to the detector. So far, the automatic config file generator doesn't add colours so that needs to be added and there needs to be an option for the default colours to be used if the user doesn't input any colours.
…g isn't provided by the user for the subdetector, the colour of the subdetector is set to the original automatic colouring so that its easy for the user to change certain colours if they want to without needing to do every subdetector
…g file that is produced so that it doesn't need to be changed too much when editing the config file
…s subdetector if it was hidden
README.md
Outdated
|
||
The script can also be run by inputing a configuration file, a root file or both. The converter automatically saves the files root, gltf and config files in automatic folders named: | ||
|
||
* `root_files` -root files can choose to be delted at the end of the run if the user wishes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `root_files` -root files can choose to be delted at the end of the run if the user wishes to | |
* `root_files` -root files can choose to be deleted at the end of the run if the user wishes to |
README.md
Outdated
|
||
To run the python script in the bin folder with only a compact file: | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
```bash |
|
||
The user can input where they'd like to save the files: | ||
|
||
python conversion_xml2gltf_autoConfig.py -cm <your-detector-file.xml> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python conversion_xml2gltf_autoConfig.py -cm <your-detector-file.xml> | |
```bash | |
python conversion_xml2gltf_autoConfig.py -cm <your-detector-file.xml> |
|
||
The user can also define how many layers of the detector they would like to see and if they would like to hide any parts of the detector: | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
```bash |
bin/configfile_generator.py
Outdated
for s in subdetector_dict: | ||
find_subdetector = re.search(f'{s}', f'{subdetector}',re.IGNORECASE) | ||
if find_subdetector: return subdetector_dict[s] | ||
if find_subdetector == None: return [0.57,0.63,0.81] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if find_subdetector == None: return [0.57,0.63,0.81] | |
return [0.57,0.63,0.81] |
This if
condition is not necessary here, because in any case where you find a suitable subdetector, you return before this line is reached. So if we come here, we can simply return the default values here.
Where do the defaults come from?
bin/configfile_generator.py
Outdated
if depth > maxDepth: tree(child, depth, maxDepth) | ||
else: dictionary = tree(child, depth, maxDepth); nd.update({raw_name: dictionary}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if depth > maxDepth: tree(child, depth, maxDepth) | |
else: dictionary = tree(child, depth, maxDepth); nd.update({raw_name: dictionary}) | |
if depth > maxDepth: | |
tree(child, depth, maxDepth) | |
else: | |
dictionary = tree(child, depth, maxDepth) | |
nd.update({raw_name: dictionary}) |
Makes it a bit easier to read.
bin/configfile_generator.py
Outdated
outer_list.append(0.8) | ||
|
||
## Adds automatic ILD coloring if the user asks | ||
if coloring == "ild": color = add_ild_colors(k); outer_list.append(color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if coloring == "ild": color = add_ild_colors(k); outer_list.append(color) | |
if coloring == "ild": | |
color = add_ild_colors(k) | |
outer_list.append(color) |
bin/phoenixExport.js
Outdated
|
||
//for (var mat of gltf["materials"]) { | ||
//console.log(mat); | ||
//} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//for (var mat of gltf["materials"]) { | |
//console.log(mat); | |
//} | |
(assuming this was mainly for debugging)
bin/phoenixExport.js
Outdated
@@ -66,10 +66,69 @@ function cleanupGeometry(node, | |||
} | |||
} | |||
|
|||
//function to generate alternative colouring that the user can set within the config file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a brief explanation in English / bullet points on what this function does on a high level, i.e. don't describe what it does by following the code, but rather what its effects are in the end, and potentially also why things are done this way.
IIUC: we are assigning colors to the material
s that we find in the gltf (resp. to some property of that)?
Created gltf folder
Completed full conversion of .xml -> .gltf with the need for a config file to speed up the conversion process. This contains the changes from #1.