Skip to content

Change the MDI Version of the ILD FCCee Models to MDI_o1_v01 #428

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

Merged
merged 2 commits into from
Apr 8, 2025

Conversation

Victor-Schwan
Copy link
Contributor

On @BrieucF 's recommendation, the default MDI version of the ILD FCCee models is changed to MDI_o1_v01.

BEGINRELEASENOTES

  • change the MDI version of the ILD FCCee models to MDI_o1_v01
  • minor: remove some trailing whitespaces in the ILD FCCee config files

ENDRELEASENOTES

@Victor-Schwan
Copy link
Contributor Author

fyi @danieljeans

@Victor-Schwan
Copy link
Contributor Author

checks fail because the stl files for MDI_o1_v01 are not available in the test environment

@atolosadelgado
Copy link
Collaborator

Hi @Victor-Schwan,

It seems that the release currently does not provide the CAD files necessary to build the fancy beampipe, which is causing the tests for ILD to fail. To address this, I would suggest one of the following options:

  • Consider disabling the CAD file option for the ILD model used in the tests. You could still include additional ILD models that aren't tested, but rely on the MDI model.
  • Alternatively, we could remove the ILD test from the ctest, though I understand this may not be the most ideal solution.

Let me know your thoughts, and I'd be happy to discuss further.

@BrieucF
Copy link
Contributor

BrieucF commented Jan 31, 2025

Or we enable the cmake option INSTALL_BEAMPIPE_STL_FILES by default?

@tmadlener
Copy link
Contributor

key4hep/key4hep-actions#9 would make it possible to easily pass that additional option for the CI.

@Victor-Schwan
Copy link
Contributor Author

I am in favor of adapting the tests such that the cmake option INSTALL_BEAMPIPE_STL_FILES is enabled by default as soon as key4hep/key4hep-actions#9 has been merged

@Victor-Schwan Victor-Schwan force-pushed the mdi_v1 branch 2 times, most recently from fd2db22 to 3a2b44b Compare February 6, 2025 16:36
@atolosadelgado
Copy link
Collaborator

Hi @Victor-Schwan,

I ran the test for ILD, which is currently failing. In addition to the missing files, I noticed another issue: when initializing the geometry, Geant4 detects an error in the beampipe CAD. Specifically, a facet is incorrectly oriented at z = +/-1111 mm, triggering the following warning message:

6: -------- WWWW ------- G4Exception-START -------- WWWW -------
6: *** G4Exception : GeomSolids1001
6:       issued by : G4TessellatedSolid::SetSolidClosed()
6: Defects in solid: "Componente1"_shape_0xd27e530 - some facets have wrong orientation!
6: *** This is just a warning message. ***
6: -------- WWWW -------- G4Exception-END --------- WWWW -------

6: -------- WWWW ------- G4Exception-START -------- WWWW -------
6: *** G4Exception : GeomSolids1001
6:       issued by : G4TessellatedSolid::SetSolidClosed()
6: Defects in solid: "Componente1"_shape_0xd2521d0 - some facets have wrong orientation!
6: *** This is just a warning message. ***
6: -------- WWWW -------- G4Exception-END --------- WWWW -------

Although this is only a warning, the test fails due to the following property in CMakeLists.txt:

SET_TESTS_PROPERTIES( t_${test_name} PROPERTIES FAIL_REGULAR_EXPRESSION "Exception;EXCEPTION;ERROR;Error" )

I think @aciarma is investigating the issue. In the meantime, could we consider modifying the ctest configuration to avoid failing on this Geant4 warning?

Let me know your thoughts :)

@aciarma
Copy link
Contributor

aciarma commented Feb 7, 2025

Hello, yes I am investigating the problem of the wrongly oriented facets, but it seems like there is not much to do when exporting the STL file from CAD. This does not seem to have an impact on the tracking of particles, but if we find a solution or a fix I will update the models

@tmadlener
Copy link
Contributor

I have added the necessary configuration to make the CI download the STL beampipe files on the fly.

@atolosadelgado
Copy link
Collaborator

atolosadelgado commented Mar 26, 2025

Hi @Victor-Schwan

thank for the modifications!

To filter out the Geant4 warning message caused by bad facets in the tessellated solid, I think it is fine if you place the following python script in test/scripts,

import sys

# Define variables for the number of lines before and after the match
lines_before = 3  # Number of lines to print before the match
lines_after = 2     # Number of lines to print after the match

buffer = []            # String buffer, size between 0 and 'lines_before'
skip = 0

for line in sys.stdin:

    # if pattern is found, clear buffer and set the number of lines to skip in the following if
    if "some facets have wrong orientation!" in line:
        buffer.clear()         # Clear the buffer before the matched line
        skip = lines_after  # skip the specified number of lines after the match
        continue
    
    # skip a number of lines after finding a match
    if skip > 0:
        skip -= 1
        continue

    buffer.append(line)
    
    # if buffer has enough lines, pop first and print it
    if len(buffer) > lines_before:
        sys.stdout.write(buffer.pop(0))  # Print the earliest line in the buffer

and modify your tests as follows:

SET( test_name "test_ILD_FCCee_v02" )
ADD_TEST( t_${test_name} sh -c "
  ${CMAKE_INSTALL_PREFIX}/bin/run_test_${PackageName}.sh ddsim --compactFile=${CMAKE_CURRENT_SOURCE_DIR}/../FCCee/ILD_FCCee/compact/ILD_FCCee_v02/ILD_FCCee_v02.xml --runType=batch -G -N=1 --outputFile=testILD_FCCee_v02.slcio 2>&1 |
python3 ${CMAKE_CURRENT_SOURCE_DIR}/scripts/filter_G4Tessellated_warnings.py"
)

I think the sh -c is needed to pass the ddsim .. | python 3 filter.py command as a single argument, otherwise the pipe seems to not work.

I have tested locally test_ILD_FCCee_v02 and it seems to work as expected: it prints everything except the warning message associated to the bad facets.

If you or anyone else have a better solutions, please go ahead :)

@tmadlener
Copy link
Contributor

Moved the ILC_Main_Crossing_Angle from top_defs to basic_defs to avoid a clash for the FCCee models, where this is defined somewhere else. This is the solution that we found requires the least changes to existing models, while still allowing us to maintain a single source of truth for all the shared bits.

Additionally, I have added setting K4GEO to the install directory of this CI run for some tests, because otherwise tests that want to use the common MDI will look for them on cvmfs where they are not installed (for the nightlies). Also, I think this is just the right thing to do (TM), because if someone changes STL files we want to catch it before they get installed everywhere.

@Victor-Schwan
Copy link
Contributor Author

@atolosadelgado thanks for the detailed and tested solution for the bad facets

@Victor-Schwan
Copy link
Contributor Author

As the bad facets in the tessellated solid might be solved at some point, I suggest not squashing the whole PR, but merging at least two commits. The idea is to have the fix for the failing tests in a separate commit, such that it can be easily reverted once a fixed geometry is available.

@atolosadelgado
Copy link
Collaborator

thanks @Victor-Schwan !

@andresailer @jmcarcell do you know how to address Victor request? otherwise we can fix manually the test once the MDI geometry is free of bad facets

@andresailer
Copy link
Contributor

andresailer commented Apr 1, 2025

do you know how to address Victor request? otherwise we can fix manually the test once the MDI geometry is free of bad facets

What is the request exactly?
And can't Victor just squash whatever commits he wants?

@atolosadelgado
Copy link
Collaborator

haha I do not know, I thought it was our duty

@Victor-Schwan can you squash as you suggested?

@Victor-Schwan Victor-Schwan force-pushed the mdi_v1 branch 2 times, most recently from 8b846e7 to 856a60a Compare April 2, 2025 17:18
@Victor-Schwan
Copy link
Contributor Author

Sure, I can do it. I was just waiting to see if there are comments on stuff to change which is usually easier before squashing. The part that I cannot do, is not squashing the PR when merging it, hence my message :)

Make sure to download STL files for CI

fix redefinition of ILD_Main_Crossing_Angle for the ILD_FCCee models

Move crossing angle definition to basic_defs

Nested includes do not work for constants and basic_defs seems to be the
best choice to avoid issues with inter dependencies between the
different constants. This makes things work with minimal changes to
existing compact files
@tmadlener
Copy link
Contributor

anyone against merging this? Are we still waiting on something else?

@atolosadelgado
Copy link
Collaborator

I can merge if you think it is ready :)

@atolosadelgado atolosadelgado merged commit f56f806 into key4hep:main Apr 8, 2025
7 checks passed
@Victor-Schwan Victor-Schwan deleted the mdi_v1 branch April 8, 2025 12:59
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.

6 participants