Geant4 parser code decompositon #254
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Geant4 parser code into a more modular architecture by decomposing the monolithic Geant4MacroGenerator class and parser methods into separate builder and parser modules. The refactoring improves code organization and maintainability.
Key Changes:
- Extracted GDML generation logic into separate
gdml/module with dedicated builders for materials, solids, and structure - Decomposed macro generation into separate parsers for beam, scoring, histograms, run, and results in
macro/module - Moved particle and quantity mappings to a centralized
constants.pyfile
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
converter/geant4/parser.py |
Simplified to delegate GDML and macro generation to new builder classes |
converter/geant4/constants.py |
Centralized GEANT4_PARTICLE_MAP and GEANT4_QUANTITY_MAP constants |
converter/geant4/gdml/builder.py |
Main GDML generation orchestrator with XML prettification |
converter/geant4/gdml/materials.py |
Material collection and emission logic |
converter/geant4/gdml/solids.py |
Solid geometry element generation |
converter/geant4/gdml/structure.py |
Structure and volume hierarchy generation |
converter/geant4/macro/builder.py |
Main macro generation orchestrator |
converter/geant4/macro/beam_parser.py |
Beam configuration parsing |
converter/geant4/macro/scoring_parser.py |
Scoring detector and quantity parsing |
converter/geant4/macro/histogram_parser.py |
Histogram output generation for probes |
converter/geant4/macro/run_parser.py |
Run command generation |
converter/geant4/macro/result_parser.py |
Result collection commands |
converter/geant4/Geant4MacroGenerator.py |
Removed in favor of modular parsers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| low = beam.get("energyLowCutoff", 0) * scale | ||
|
|
||
| return [ | ||
| f"/gps/direction {direction[0]} {direction[1]} {direction[2]}", |
There was a problem hiding this comment.
This line doesn't belong here
|
A general remark - why a simple code, like in Geant4GDMLBuilder is not a method ?
should do the job as well. The same applies to The code looks very weird and not pythonic :/ |
Fair enough, on daily basis i write in java so the code could have some OOP similarities,. Do you want more method base approach ? |
Yes, Python is not a Java. I ran quick copilot survey on this code which generated guidelines which I strongly support: In python we don't care much about private/public stuff (everything is public anyway). We don't inherit much as well. What requires more attention that many methods/function in this code have way too many lines and should be splitted into smaller pieces. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 16 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
grzanka
left a comment
grzanka
left a comment
There was a problem hiding this comment.
I would recommend to avoid as much as possible a coding style where you have a function which modify the parameters they take.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 18 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
grzanka
left a comment
grzanka
left a comment
There was a problem hiding this comment.
Can you convert all of your codes into simple functions, with meaningful names, which do not modify its arguments. Good idea would be also to completely get rid of classes.
We don't need a polymorphism here, nor any need to keep a state of some object. The logic here is simple transformation from one state to another which can be achieved by set of testable functions scattered among couple of modules.
we have only one class that is absolutely needed, ( extends class parser as every other converter ) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 18 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 11: { | ||
| "name": "geantino", | ||
| "allowed_units": ["MeV"], | ||
| "target_unit": "MeV" | ||
| }, |
There was a problem hiding this comment.
Particle IDs 3 and 11 both map to the same particle name "geantino" with identical configurations. This appears to be a duplicate entry that could cause confusion or unexpected behavior. If both IDs are intended to map to the same particle, consider adding a comment explaining why; otherwise, one entry may be incorrect.
|
|
||
|
|
||
| def _append_beam_shape(lines: List[str], beam: Dict[str, Any]) -> None: | ||
| """Append commands describing the beam's spatial distribution.""" |
There was a problem hiding this comment.
The docstring says "Append commands describing the beam's spatial distribution" but this function modifies the passed lines list in-place rather than returning anything. Consider either returning the commands and having the caller extend the list, or clarifying in the docstring that this function mutates the input list.
| """Append commands describing the beam's spatial distribution.""" | |
| """Append to ``lines`` the commands describing the beam's spatial distribution.""" |
| lines: List[str], | ||
| probe_histograms: List[Dict[str, Any]], | ||
| ) -> None: | ||
| """Append a scoring quantity definition to the macro.""" |
There was a problem hiding this comment.
The docstring says "Append a scoring quantity definition to the macro" but this function modifies the passed lines and probe_histograms lists in-place. The docstring should clarify that this function mutates the input lists, or the function should be refactored to return values for consistency.
| """Append a scoring quantity definition to the macro.""" | |
| """Append a scoring quantity definition to the macro, mutating ``lines`` and | |
| ``probe_histograms`` in-place as needed.""" |
There was a problem hiding this comment.
@Ciorolla Can't you simply redesign this function so it is simply returning list of lines to be appended ?
mutability is often source of many errors.
| def parse_configs(self, json_data: dict) -> None: | ||
| """Parse the provided JSON configuration and generate GDML content.""" | ||
| """Parse full JSON configuration into internal GDML and macro builders.""" | ||
| world = None |
There was a problem hiding this comment.
The variable initialization world = None is unnecessary since it's immediately reassigned in the conditional block below. The code can be simplified by initializing world directly inside the if statement or using a ternary expression.
| configs_json = super().get_configs_json() | ||
| configs_json.update({ | ||
| """Return the full configuration JSON including generated GDML and macro data.""" | ||
| cfg = super().get_configs_json() |
There was a problem hiding this comment.
The variable name cfg is abbreviated and less clear than the previous configs_json. For consistency with the method name get_configs_json() and improved readability, consider using a more descriptive name like configs_json or result.
|
|
||
| DEFAULT_PARTICLE_ID: int = 2 | ||
| DEFAULT_PARTICLE_A: int = 1 | ||
| DEFAULT_PARTICLE_Z_FROM_A: bool = True |
There was a problem hiding this comment.
The constant DEFAULT_PARTICLE_Z_FROM_A is defined but never used in the codebase. This appears to be dead code that should either be utilized or removed.
| DEFAULT_PARTICLE_Z_FROM_A: bool = True |
|
|
||
|
|
||
| def generate_beam_lines(data: Dict[str, Any]) -> List[str]: | ||
| """Parse the beam configuration and append GEANT4 /gps commands.""" |
There was a problem hiding this comment.
The docstring states "Parse the beam configuration and append GEANT4 /gps commands" but the function returns a list of strings rather than appending to an existing list. The docstring should be updated to accurately reflect that this function generates and returns a list of commands.
| """Parse the beam configuration and append GEANT4 /gps commands.""" | |
| """Parse the beam configuration and return GEANT4 /gps commands as a list of strings.""" |
|
|
||
| class Geant4Parser(Parser): | ||
| """Parser that converts JSON to GDML format for geant4 simulations""" | ||
| """Parser that converts JSON simulation configurations into GDML geometry and GEANT4 macro scripts.""" |
There was a problem hiding this comment.
The docstring describes parsing "JSON simulation configurations into GDML geometry and GEANT4 macro scripts" but should use "Geant4" (with lowercase 'eant4') to match the standard capitalization used elsewhere in the codebase, including the class name itself "Geant4Parser".
| """Parser that converts JSON simulation configurations into GDML geometry and GEANT4 macro scripts.""" | |
| """Parser that converts JSON simulation configurations into GDML geometry and Geant4 macro scripts.""" |
| lines: List[str], | ||
| probe_histograms: List[Dict[str, Any]], | ||
| ) -> None: | ||
| """Append all scoring quantities and filters for a given detector.""" |
There was a problem hiding this comment.
The docstring says "Append all scoring quantities and filters for a given detector" but this function modifies the passed lines and probe_histograms lists in-place rather than returning anything. The docstring should clarify that this function mutates the input lists, or consider refactoring to return values for consistency with other parser functions.
| """Append all scoring quantities and filters for a given detector.""" | |
| """Append all scoring quantities and filters for a given detector. | |
| This function mutates the provided ``lines`` and ``probe_histograms`` | |
| lists in-place by appending the corresponding scoring macro commands. | |
| It does not return any value. | |
| """ |
|
|
||
| def _append_mesh_lines(detector: Dict[str, Any], geom_type: str, params: Dict[str, Any], | ||
| pos_det: List[float], lines: List[str]) -> None: | ||
| """Append a mesh-type scoring detector definition to the macro.""" |
There was a problem hiding this comment.
The docstring says "Append a mesh-type scoring detector definition to the macro" but this function modifies the passed lines list in-place. The docstring should clarify that this function mutates the input list, or the function should be refactored to return the lines for consistency.
| """Append a mesh-type scoring detector definition to the macro.""" | |
| """Append a mesh-type scoring detector definition to the macro by mutating the given lines list.""" |
No description provided.