Nested material parametrization for voxellized geometry#41
Nested material parametrization for voxellized geometry#41sethrj merged 16 commits intoceleritas-project:mainfrom
Conversation
…trization.test.cc - Implemented ICRP110PhantomMaterial_Female class to define materials for the female phantom model. - Created ICRP110PhantomNestedParameterisation class to handle voxel parameterisation and material assignment.
sethrj
left a comment
sethrj
left a comment
There was a problem hiding this comment.
Thanks for this @michele-colle! Would it be possible for you to simplify the example even further? The goal is to show with the least amount of code that we're missing something important, and then find the next step to the basic capability of material-dependent parameterization.
- I think you can get rid of the phantom materials. All we really need is hydrogen at 8 different densities (separate G4Material objects).
- Similarly, you should be able to delete the visualization-related code.
Superficial changes that should be made before this is merged (lower priority)
- Install the
pre-commitutility so that the code will be formatted consistently with the rest of the repository? Then runpre-commit run --all-filesto update the formatting of the C++ files. - Revert the reformatting of CMakePresets.json
- Delete commented-out code, or if it should remain in there, replace
//(signifying textual comments) by#if 0/#endifpairs (signifying disabled code) with a comment explaining why it's there but disabled
I hope it's not too much of a burden, but that will help improve the readability of the changes and make it easier to review. Thanks for bringing it to this point already!
|
Ok, I have deleted the parts of code related to the visualization and the material generation files, now the materials are the 7 requested hydrogen densities. An error has occurred: InvalidManifestError:
==> File /home/colle/.cache/pre-commit/repofji9xiwl/.pre-commit-hooks.yaml
==> At Hook(id='clang-format')
==> At key: types_or
==> At index 10
=====> Type tag 'metal' is not recognized. Try upgrading identify and pre-commit?
Check the log at /home/colle/.cache/pre-commit/pre-commit.log
colle@colle-HP-Z2-Tower-G9-Workstation-Desktop-PC:~/Desktop/g4vg/g4vg$ |
|
Remove 'metal' from the .yml files under /home/colle/.cache/pre-commit, and re-run the command that shows this error. |
thanks! it worked! |
Prompt: > The nested parameterization code needs some major cleanup: please use Custom.test.cc as a "best practices" baseline. Although there are some unusual memory semantics dictated by the underlying Geant4 API, please: > > - use C++17 best practices where possible > - use std::array<int, 3> instead of x,y,z sequences of numbers > - use vectors instead of raw pointers where possible > - update names to be consistent with Custom.test.cc > - Remove superfluous comments, and use inline variable comments such as /* rotation = */ instead of a end-of-line comment > - Move construction of the paramterization (vector, dimensions) into the construction, and delete the setters
|
Hi @michele-colle I'm unable to upload fixes this PR because (I think?) the pull request is from your "main" branch rather than a topic branch. Could you please run the following commands in your local development repository: git remote add -f sethrj https://github.com/sethrj/g4vg
git merge --ff-only sethrj/pr-41-fixes
git push
git remote rm sethrj |
4 participants
Based on the ICRUPhantom example:
https://gitlab.cern.ch/geant4/geant4/-/tree/master/examples/advanced/ICRP110_HumanPhantoms
I have added a minimal test of a voxellized 2x3x5 geometry with 7 material (plus air) in order to verify the correctness of the geometry creation.