Skip to content

Conversation

@KoljaFrahm
Copy link
Contributor

@KoljaFrahm KoljaFrahm commented Nov 9, 2025

This Pull request:

Adds the option of exporting auxiliary of TGeoVolumes in TGDMLWrite
Uses same format as in https://github.com/root-project/root/blob/master/geom/gdml/src/TGDMLParse.cxx

Changes or fixes:

Fixes #20356

Checklist:

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

This PR fixes #20356

@KoljaFrahm
Copy link
Contributor Author

I reformatted my commit (clang), do I still have to do so something to start the tests again?

@dpiparo
Copy link
Member

dpiparo commented Nov 12, 2025

Is there a way in which we could test this fix?

@KoljaFrahm
Copy link
Contributor Author

KoljaFrahm commented Nov 12, 2025

I described two ways in the issues (Reproducer).
Is this enough or should I describe something in more detail?

@KoljaFrahm
Copy link
Contributor Author

I am not sure why the ruff code analysis fails for files I haven't changed

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.

Thanks @KoljaFrahm for this addition - reading the auxiliary data via TGDMLParse was only intended so far to provide user access to these data via the volume user extensions, but it is indeed nice to be able to re-export it. See my comment requesting a small correction - otherwise your patch works nicely!

@agheata
Copy link
Member

agheata commented Nov 12, 2025

Is there a way in which we could test this fix?

Actually, this is not a fix but a feature, allowing the re-export of GDML auxiliary volume data imported from a GDML file. It could be tested eventually if we make a more general GDML import/export regression test

This adds the option of exporting auxiliary of TGeoVolumes in TGDMLWrite
Fixes root-project#20356
@KoljaFrahm
Copy link
Contributor Author

Adjusted and rebased :)

@KoljaFrahm KoljaFrahm requested a review from agheata November 12, 2025 15:56
@KoljaFrahm
Copy link
Contributor Author

KoljaFrahm commented Nov 19, 2025

Is still something missing from my side? Would be nice to have this in the next root version :)

@dpiparo
Copy link
Member

dpiparo commented Nov 28, 2025

I propose to have these changes integrated in the main branch as soon as the tests passes.
Then, if @agheata agrees, we could potentially think of having it integrated in the v6-38-00-patches branch to have it available in 6.38.02, likely available in a few weeks from now...

@KoljaFrahm
Copy link
Contributor Author

That would be great!

@github-actions
Copy link

Test Results

    22 files      22 suites   3d 20h 27m 42s ⏱️
 3 777 tests  3 775 ✅ 0 💤 2 ❌
81 184 runs  81 182 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 30ef55c.

@guitargeek guitargeek merged commit 74e607e into root-project:master Nov 28, 2025
26 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auxiliaries of volumes are not exportet in gdml

4 participants