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.

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

feat: support TGeoTessellated volumes in DetectorChecksum #1141

Merged
merged 12 commits into from
Jul 13, 2023

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Jul 5, 2023

This adds support for TGeoTessellated in detector checksumming. It also (necessarily) exports them to gdml. This requires a const_cast because const access to vertices and facets was only recently enabled. Also added a test on an existing TGeoTessellated shape check.

BEGINRELEASENOTES

  • Support TGeoTessellated volumes in DetectorChecksum

ENDRELEASENOTES

Copy link
Contributor Author

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

Testing with geoPluginRun -input examples/ClientTests/compact/Check_Shape_Tessellated.xml -plugin DD4hepDetectorChecksum -dump_solids -debug 4 indicates some remaining issues...

DDCore/src/plugins/DetectorChecksum.cpp Outdated Show resolved Hide resolved
DDCore/src/plugins/DetectorChecksum.cpp Outdated Show resolved Hide resolved
@wdconinc
Copy link
Contributor Author

wdconinc commented Jul 6, 2023

This now produces the following gdml:

gdml
<define>
<position name"_v0 lunit="mm" x="0.000000" y="1.618034" z="-1.000000"/>
<position name"_v1 lunit="mm" x="0.000000" y="0.618034" z="-1.618034"/>
<position name"_v2 lunit="mm" x="-1.000000" y="0.000000" z="-1.618034"/>
<position name"_v3 lunit="mm" x="-1.000000" y="1.000000" z="-1.000000"/>
<position name"_v4 lunit="mm" x="1.000000" y="1.000000" z="-1.000000"/>
<position name"_v5 lunit="mm" x="1.000000" y="0.000000" z="-1.618034"/>
<position name"_v6 lunit="mm" x="0.618034" y="1.618034" z="0.000000"/>
<position name"_v7 lunit="mm" x="1.618034" y="1.000000" z="0.000000"/>
<position name"_v8 lunit="mm" x="-0.618034" y="1.618034" z="0.000000"/>
<position name"_v9 lunit="mm" x="0.000000" y="1.618034" z="1.000000"/>
<position name"_v10 lunit="mm" x="-1.618034" y="1.000000" z="0.000000"/>
<position name"_v11 lunit="mm" x="1.618034" y="0.000000" z="-0.618034"/>
<position name"_v12 lunit="mm" x="1.618034" y="-1.000000" z="0.000000"/>
<position name"_v13 lunit="mm" x="1.000000" y="-1.000000" z="-1.000000"/>
<position name"_v14 lunit="mm" x="0.000000" y="-1.618034" z="-1.000000"/>
<position name"_v15 lunit="mm" x="0.000000" y="-0.618034" z="-1.618034"/>
<position name"_v16 lunit="mm" x="1.000000" y="1.000000" z="1.000000"/>
<position name"_v17 lunit="mm" x="1.000000" y="0.000000" z="1.618034"/>
<position name"_v18 lunit="mm" x="1.618034" y="0.000000" z="0.618034"/>
<position name"_v19 lunit="mm" x="-1.000000" y="1.000000" z="1.000000"/>
<position name"_v20 lunit="mm" x="-1.000000" y="0.000000" z="1.618034"/>
<position name"_v21 lunit="mm" x="0.000000" y="0.618034" z="1.618034"/>
<position name"_v22 lunit="mm" x="0.000000" y="-0.618034" z="1.618034"/>
<position name"_v23 lunit="mm" x="0.000000" y="-1.618034" z="1.000000"/>
<position name"_v24 lunit="mm" x="1.000000" y="-1.000000" z="1.000000"/>
<position name"_v25 lunit="mm" x="-1.618034" y="0.000000" z="0.618034"/>
<position name"_v26 lunit="mm" x="-1.000000" y="-1.000000" z="1.000000"/>
<position name"_v27 lunit="mm" x="-1.618034" y="-1.000000" z="0.000000"/>
<position name"_v28 lunit="mm" x="-1.618034" y="0.000000" z="-0.618034"/>
<position name"_v29 lunit="mm" x="-1.000000" y="-1.000000" z="-1.000000"/>
<position name"_v30 lunit="mm" x="-0.618034" y="-1.618034" z="0.000000"/>
<position name"_v31 lunit="mm" x="0.618034" y="-1.618034" z="0.000000"/>
</define>
<tessellated name="">
<quadrangular vertex1="_v0" vertex2="_v1" vertex3="_v2" vertex4="_v3" type="ABSOLUTE"/>
<quadrangular vertex1="_v4" vertex2="_v5" vertex3="_v1" vertex4="_v0" type="ABSOLUTE"/>
<quadrangular vertex1="_v4" vertex2="_v0" vertex3="_v6" vertex4="_v7" type="ABSOLUTE"/>
<quadrangular vertex1="_v8" vertex2="_v9" vertex3="_v6" vertex4="_v0" type="ABSOLUTE"/>
<quadrangular vertex1="_v8" vertex2="_v0" vertex3="_v3" vertex4="_v10" type="ABSOLUTE"/>
<quadrangular vertex1="_v4" vertex2="_v7" vertex3="_v11" vertex4="_v5" type="ABSOLUTE"/>
<quadrangular vertex1="_v11" vertex2="_v12" vertex3="_v13" vertex4="_v5" type="ABSOLUTE"/>
<quadrangular vertex1="_v13" vertex2="_v14" vertex3="_v15" vertex4="_v5" type="ABSOLUTE"/>
<quadrangular vertex1="_v5" vertex2="_v15" vertex3="_v2" vertex4="_v1" type="ABSOLUTE"/>
<quadrangular vertex1="_v6" vertex2="_v9" vertex3="_v16" vertex4="_v7" type="ABSOLUTE"/>
<quadrangular vertex1="_v7" vertex2="_v16" vertex3="_v17" vertex4="_v18" type="ABSOLUTE"/>
<quadrangular vertex1="_v11" vertex2="_v7" vertex3="_v18" vertex4="_v12" type="ABSOLUTE"/>
<quadrangular vertex1="_v8" vertex2="_v10" vertex3="_v19" vertex4="_v9" type="ABSOLUTE"/>
<quadrangular vertex1="_v9" vertex2="_v19" vertex3="_v20" vertex4="_v21" type="ABSOLUTE"/>
<quadrangular vertex1="_v16" vertex2="_v9" vertex3="_v21" vertex4="_v17" type="ABSOLUTE"/>
<quadrangular vertex1="_v22" vertex2="_v17" vertex3="_v21" vertex4="_v20" type="ABSOLUTE"/>
<quadrangular vertex1="_v22" vertex2="_v23" vertex3="_v24" vertex4="_v17" type="ABSOLUTE"/>
<quadrangular vertex1="_v18" vertex2="_v17" vertex3="_v24" vertex4="_v12" type="ABSOLUTE"/>
<quadrangular vertex1="_v20" vertex2="_v19" vertex3="_v10" vertex4="_v25" type="ABSOLUTE"/>
<quadrangular vertex1="_v26" vertex2="_v20" vertex3="_v25" vertex4="_v27" type="ABSOLUTE"/>
<quadrangular vertex1="_v22" vertex2="_v20" vertex3="_v26" vertex4="_v23" type="ABSOLUTE"/>
<quadrangular vertex1="_v27" vertex2="_v25" vertex3="_v10" vertex4="_v28" type="ABSOLUTE"/>
<quadrangular vertex1="_v27" vertex2="_v28" vertex3="_v2" vertex4="_v29" type="ABSOLUTE"/>
<quadrangular vertex1="_v14" vertex2="_v30" vertex3="_v27" vertex4="_v29" type="ABSOLUTE"/>
<quadrangular vertex1="_v30" vertex2="_v23" vertex3="_v26" vertex4="_v27" type="ABSOLUTE"/>
<quadrangular vertex1="_v3" vertex2="_v2" vertex3="_v28" vertex4="_v10" type="ABSOLUTE"/>
<quadrangular vertex1="_v14" vertex2="_v29" vertex3="_v2" vertex4="_v15" type="ABSOLUTE"/>
<quadrangular vertex1="_v14" vertex2="_v31" vertex3="_v23" vertex4="_v30" type="ABSOLUTE"/>
<quadrangular vertex1="_v13" vertex2="_v12" vertex3="_v31" vertex4="_v14" type="ABSOLUTE"/>
<quadrangular vertex1="_v12" vertex2="_v24" vertex3="_v23" vertex4="_v31" type="ABSOLUTE"/>
</tessellated>

with the following notes:

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Test Results

       6 files         6 suites   7h 26m 33s ⏱️
   358 tests    358 ✔️ 0 💤 0
1 058 runs  1 058 ✔️ 0 💤 0

Results for commit c99e559.

♻️ This comment has been updated with latest results.

@MarkusFrankATcernch
Copy link
Contributor

MarkusFrankATcernch commented Jul 6, 2023

@wdconinc
I fully agree with your proposal, but there is a very good chance checksums for non-trival tessellated shapes will never work:
There is a real chance that the checksum creation runs into numerical problems and with many, many facets some digit differs.
I am not sure what to do in such a case, but doing the obvious (what you implemented) might not work.....
This feature already shows up in the shape tests, where tessellated shapes are tested via the mesh dump (which is not really different from what you did).

@wdconinc
Copy link
Contributor Author

wdconinc commented Jul 6, 2023

there is a very good chance checksums for non-trival tessellated shapes will never work

That is a good point. Since the checksum creation is not intended to be a persistency model (even though we use gdml as a step), would we be ok with explicitly limiting the numerical precision with which we write the vertex positions?

Another option is to limit the checksum to the mesh topology, without any positions at all. That would be a more limited test but for automatically generated hashes may still be sufficient.

I would primarily like this feature (in a way that is numerically stable) because it is currently not possible to get a checksum for a geometry with tessellated solids (which we use for one detector volume in our EIC geometry).

@MarkusFrankATcernch
Copy link
Contributor

@wdconinc
I do not know which approach is the best. I only know things are very touchy and many CAD models e.g. have degenerate facets with hardly any area. Even the number of vertices depends often on some precision....
Hence I do not really know if the details of the checksum of tessellated shapes should be part of the detector checksum.
However, something must be put in place to represent the tessellated shape and if it is only the name tag.

@wdconinc
Copy link
Contributor Author

wdconinc commented Jul 6, 2023

It is true that there are many imperfect meshing solutions out there (and there's a list of other caveats with meshes that I keep having to point people to).

But isn't that out of scope of the checksum and of the issue of numerical stability? We are not aiming to determine a checksum that is identical for two meshes exported from the same CAD model; we are just aiming to determine a checksum that is the same for the same exported mesh, on multiple architectures, in multiple systems of units, and in the face of numerical stability when reading in the mesh. If that includes degenerate facets (or missing facets, another common one), then the checksum shouldn't second-guess the mesh and do something that is reproducible.

if it is only the name tag

I haven't looked into the reason for the comment at https://github.com/AIDASoft/DD4hep/blob/master/DDCore/src/plugins/DetectorChecksum.cpp#L182, but I assume the name is turned into <name>0x012345678 or something? Could we just chop off the pointer again from the name? Or is this a harder issue?

@MarkusFrankATcernch
Copy link
Contributor

@wdconinc Normally in dd4hep shapes and placements do not have a user defined name.
You construct named volumes out of a shape and a material. Since shapes are not intended to be shared across wide areas (e.g. between subdetectors), a shape name was felt to be only an additional complication.
For placements it is ROOT which dresses the placement with the copy number.
Hence, if you strip the pointer off, nothing is left --- and this is what is implemented.

But you are wrong about the checksums not being persistent. The checksum is actually meant to uniquely identify
a design. Hence, it should ideally be the same across platforms, compilers, even with different systems of units:
See one of your changes adding a missing unit of length normalization.
For this reason there must be some robustness in the algorithm and if tessellated shapes cannot fulfill it,
there is not a lot one can do except excluding them.

@wdconinc
Copy link
Contributor Author

wdconinc commented Jul 6, 2023

But you are wrong about the checksums not being persistent.

I am saying the checksum should be persistent: "we are just aiming to determine a checksum that is the same for the same exported mesh, on multiple architectures, in multiple systems of units, and in the face of numerical stability when reading in the mesh."

What I am also saying is that we should not expect two different meshes to have the same checksum, even if they were generated from the same CAD model. Whether the export created degenerate facets or whether the number of vertices is the same all the time are not really within the scope here. We have a certain mesh, it has a fixed number of vertices, it may have degenerate facets. If you change the mesh, the checksum must change too. (But, as above, it should be otherwise independent of other running conditions.)

Alternatively

We can also just use:

    else if ( shape->IsA() == TGeoTessellated::Class() )  {
      log << "<tessellated></tessellated>" << newline;
    }

since we don't have name and you don't want to use vertices, it seems to me. But that's just an absolute minimal solution and I think we can do better here.

@MarkusFrankATcernch
Copy link
Contributor

This would be a possibility. We could have a flag in the DetectorChecksum to switch between a extended checksum
and a simple checksum (aka your Alternative) for tessellated shapes.

@MarkusFrankATcernch
Copy link
Contributor

@wdconinc Fixing the tests is also part of the MR.... ;-)
Thank you!

@wdconinc
Copy link
Contributor Author

wdconinc commented Jul 7, 2023

Understood. Not sure yet why I get a different checksum on my local system compared to CI, even for just <tessellated></tessellated>... Hope to complete today.

@MarkusFrankATcernch
Copy link
Contributor

Well....At some point you hash the string produced.
The whole algorithm is only invariant if the string is identical.

@wdconinc wdconinc marked this pull request as ready for review July 13, 2023 00:32
@wdconinc
Copy link
Contributor Author

This now passes all checks and behaves as intended, I think.

  • default checksum of tessellated solids is minimal,
  • -meshes will use meshes in the checksum,
  • checks use -precision 3 to be independent of the numerical precision in CI.

Let me know if you want a history rewrite or clean up into fewer commits.

@MarkusFrankATcernch MarkusFrankATcernch enabled auto-merge (rebase) July 13, 2023 13:37
@MarkusFrankATcernch MarkusFrankATcernch merged commit 75e88dd into AIDASoft:master Jul 13, 2023
13 of 14 checks passed
@wdconinc wdconinc deleted the checksum-tessellated branch July 13, 2023 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants