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.

Sign up for GitHub

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

Jump to bottom

GDML: Fix material properties #11888

Merged
merged 4 commits into from
Dec 15, 2022
Merged

GDML: Fix material properties #11888

merged 4 commits into from
Dec 15, 2022

Conversation

MarkusFrankATcernch
Copy link
Contributor

@MarkusFrankATcernch MarkusFrankATcernch commented Dec 13, 2022 •
edited
Loading

This Pull request:

  • Eject material property refs as first child elements as required from the GDML schema
  • Add the atomic number N to the <atom> attributes.
  • Eject const properties as matrices in GDML with one element rather than constants.
  • Consequently, when reading a 1x1 matrix, it will be declared as const property to the TGeoManager.

Changes or fixes:

Creation of GDML files understood by Geant4

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@MarkusFrankATcernch
Update TGDMLWrite.cxx
b4749cd 
Add missing conversion of const property to 1x1 GDML matrix.
@agheata
Copy link
Member

agheata commented Dec 13, 2022

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@MarkusFrankATcernch
Update TGDMLWrite.cxx
d7508f3 
Remove missing line from earlier material commit.
@agheata agheata self-assigned this Dec 14, 2022
agheata
agheata requested changes Dec 14, 2022
View reviewed changes
Copy link
Member

@agheata agheata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkusFrankATcernch there are some changes related to units and clang-format-only that should not appear, can you please remove them?

geom/gdml/src/TGDMLWrite.cxx Show resolved Hide resolved
geom/gdml/src/TGDMLWrite.cxx Outdated
@@ -728,7 +728,6 @@ XMLNodePointer_t TGDMLWrite::CreateAtomN(Double_t atom, const char * unit)
{
const TString fltPrecision = TString::Format("%%.%dg", fFltPrecision);
XMLNodePointer_t atomN = fGdmlE->NewChild(nullptr, nullptr, "atom", nullptr);
if ( gGeoManager->GetDefaultUnits() != TGeoManager::kRootUnits ) atom /= 1e19; // Correct for G4 unit system
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to the PR

geom/gdml/src/TGDMLWrite.cxx Outdated
@@ -741,7 +740,6 @@ XMLNodePointer_t TGDMLWrite::CreateDN(Double_t density, const char * unit)
{
const TString fltPrecision = TString::Format("%%.%dg", fFltPrecision);
XMLNodePointer_t densN = fGdmlE->NewChild(nullptr, nullptr, "D", nullptr);
if ( gGeoManager->GetDefaultUnits() != TGeoManager::kRootUnits ) density /= 1e16; // Correct for G4 unit system
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to the PR

@agheata
Copy link
Member

agheata commented Dec 15, 2022

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@agheata
Copy link
Member

agheata commented Dec 15, 2022

@MarkusFrankATcernch these commits should be squashed unless you want to do an interactive rebase and merge some of them manually. It does not make sense to have commits like "re-introducing bug to maximize number of commits" ;-)

@MarkusFrankATcernch
Copy link
Contributor Author

@agheata How is a "squash" done ? I am not at all a git expert....actually git is my outspoken enemy!

@agheata
Copy link
Member

agheata commented Dec 15, 2022 •
edited
Loading

@agheata How is a "squash" done ? I am not at all a git expert....actually git is my outspoken enemy!

I can do it from the git web interface when merging, but this will squash all commits into one. Otherwise, if your branch started after say commit #N, you can locally rebase interactively the branch on top of that commit using git rebase -i #N, which opens an editor and allows you to do actions like squashing commits. Then you must push the branch with --force, since you are altering history.

@MarkusFrankATcernch
Copy link
Contributor Author

MarkusFrankATcernch commented Dec 15, 2022 •
edited
Loading

@agheata In this case squashing all commits into the first one should be just perfect
There is then anyhow PR #11895 , which does nothing than just this.

@MarkusFrankATcernch
Copy link
Contributor Author

@agheata Will you do the "squash" please? I have no real clue what to type to get there ....

@agheata
Copy link
Member

agheata commented Dec 15, 2022

@agheata Will you do the "squash" please? I have no real clue what to type to get there ....

Sure, I will

@MarkusFrankATcernch
Copy link
Contributor Author

@agheata Thanks a lot!

@MarkusFrankATcernch MarkusFrankATcernch mentioned this pull request Dec 15, 2022
Merged
@agheata agheata merged commit d7c251e into root-project:master Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agheata agheata agheata approved these changes

Assignees

@agheata agheata

Labels
affects:master in:Geometry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarkusFrankATcernch @phsft-bot @agheata