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

[FCCee] Centralize beampipe geometries in an MDI folder #344

Merged
merged 26 commits into from
Jul 19, 2024

Conversation

aciarma
Copy link
Contributor

@aciarma aciarma commented Jun 26, 2024

Following issue #338

BEGINRELEASENOTES

  • added k4geo/FCCee/MDI folder
  • put the shape based beampipe in MDI_o1_v00
  • prepared k4geo/FCCee/MDI/compact/MDI_o1_v01/ which will contain the CAD beampipe
  • modified the main compact files of ALLEGRO_o1_v03 and IDEA_o1_v03 to include the centralized beampipe and prepare them for the CAD ones
  • removed HOMAbsorbers.xml from ALLEGRO_o1_v03 since they are not needed with the low impedance beam pipe.

ENDRELEASENOTES

The MDI folder will be updated with new CAD models of beam pipe and other MDI elements as they are developed (e.g. remote vacuum connection, bellows, cryostats, shieldings, magnets, supports, services ...).
Maybe the LumiCal could be moved here, as it is a common element at all the IPs.
CLD compact can be modified as well.

@aciarma
Copy link
Contributor Author

aciarma commented Jun 26, 2024

tests fail because the following exception is caught:

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


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

the same geometry has been tested and works as expected, so I think this warning can be bypassed.

I wait for confirmation anyway

@andresailer
Copy link
Contributor

Please keep something for BEGIN/END RELEASENOTES.

Copy link
Contributor

@andresailer andresailer left a comment

Choose a reason for hiding this comment

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

Some issues, some nitpicking, some request for comment.

Regarding the test:

Add a space before Exception to ignore the G4Exception warning. If an actual error occurs, geant4 prints, Exception instead of warning inside it (or it did in the past...)

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

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

FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/ALLEGRO_o1_v03.xml Outdated Show resolved Hide resolved
FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/ALLEGRO_o1_v03.xml Outdated Show resolved Hide resolved
FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/ALLEGRO_o1_v03.xml Outdated Show resolved Hide resolved
FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/materials.xml Outdated Show resolved Hide resolved
FCCee/IDEA/compact/IDEA_o1_v03/IDEA_o1_v03.xml Outdated Show resolved Hide resolved
FCCee/IDEA/compact/IDEA_o1_v03/IDEA_o1_v03.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@BrieucF BrieucF left a comment

Choose a reason for hiding this comment

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

Great Andrea, thanks a lot for this!

@BrieucF
Copy link
Contributor

BrieucF commented Jun 27, 2024

@andresailer shall we take the beampipe from there as well for CLD or you prefer to handle this differently? If yes, I would propose to do this on CLD_o2_v06 without bumping the version since it has not really been used yet.

@andresailer
Copy link
Contributor

@andresailer shall we take the beampipe from there as well for CLD or you prefer to handle this differently? If yes, I would propose to do this on CLD_o2_v06 without bumping the version since it has not really been used yet.

I would want to know how this affects simulation time, and see an overlap check

@andresailer
Copy link
Contributor

And one should also check how this affects track reconstruction, if Material is properly accounted for.

@aciarma
Copy link
Contributor Author

aciarma commented Jun 27, 2024

And one should also check how this affects track reconstruction, if Material is properly accounted for.

actually I did some tracking using these CAD files to see the energy deposit in the LumiCal by particles scattered off the beam pipe, and when the material was read uncorrectly (i.e. DefaultMaterial = steel) we noticed a large contribution from such secondaries, that mostly disappeared when fixing the bug and using actually AlBeMet or Beryllium

@BrieucF
Copy link
Contributor

BrieucF commented Jun 27, 2024

And one should also check how this affects track reconstruction, if Material is properly accounted for.

actually I did some tracking using these CAD files to see the energy deposit in the LumiCal by particles scattered off the beam pipe, and when the material was read uncorrectly (i.e. DefaultMaterial = steel) we noticed a large contribution from such secondaries, that mostly disappeared when fixing the bug and using actually AlBeMet or Beryllium

I guess Andre meant to check the performance of the track reconstruction in CLD with the new beampipe. Since some people (Leonhard/Gaelle) have workflows ready to produce those plots, maybe one could ask them to check that?

Shall we leave the inclusion of this centralized beampipe in CLD for another PR?

@BrieucF
Copy link
Contributor

BrieucF commented Jun 27, 2024

For the lumiCal (here or in another PR, as you wish), I propose to take the one from CLD_o2_v06, this is the most up-to date

@BrieucF
Copy link
Contributor

BrieucF commented Jun 27, 2024

You can ignore the overlap of the VTX with himself (IIRC Armin has fixed them in his last version, still to be merged).

For the beampipe, I see 5 overlaps, most of them of the order of 50 um, except one which is 1.4 mm. Not knowing the tool used to export the CAD to tesselated solids, I am not sure how to deal with this... Can you ask the person having the tool in hand to see if there are some "options" when doing this export?

Anyone has another idea?

The impact on physics should be small though, what do you think Andre?

@Zehvogel
Copy link
Contributor

Shall we leave the inclusion of this centralized beampipe in CLD for another PR?

yes, or at least make it CLD_o2_v07 and not a silent change.

Just out of curiosity, are the CAD files exported with a "sane" resolution this time or still Angstrom precision?
By how much does changing to the CAD beam pipe change the simulation time?

@aciarma
Copy link
Contributor Author

aciarma commented Jul 5, 2024

The time taken by the tests of #351 (last merged PR, without CAD) and #344 (this PR) seems negligible.

------ #351
14/22 Test #15: t_test_IDEA_o1_v03 ............... Passed 53.16 sec
16/22 Test #17: t_test_ALLEGRO_o1_v03 ............ Passed 55.70 sec

------ #344
13/22 Test #15: t_test_IDEA_o1_v03 ............... Passed 54.27 sec
16/22 Test #17: t_test_ALLEGRO_o1_v03 ............ Passed 58.94 sec

About the resolution, it is necessary to use small facets in order to have overlaps between touching curved surfaces as few and small as possible. There is no parameter in the tool (AutoCAD) used to export the STL to avoid these artifacts as each volume is converted by itself, and the tesselation is done by the software, without any possibility to interfere

@aciarma
Copy link
Contributor Author

aciarma commented Jul 8, 2024

It still seems to be some requested change to do but I think I did everything, what am I missing?

@BrieucF
Copy link
Contributor

BrieucF commented Jul 8, 2024

I think you indeed addressed the above comments. Can you further apply the following changes:

  • "save" the native shape based beampipe (including HOMAbsorber) in k4geo/FCCee/MDI/compact/MDI_o1_v00/? For the record and to be able to do checks in both scenarios.
  • place beamInstrumentation.xml both in k4geo/FCCee/MDI/compact/MDI_o1_v01/ and in MDI_o1_v01?
  • Add a README k4geo/FCCee/MDI/compact/README.md to book-keep the difference between beampipe implementations
  • Add a line in the READMEs: FCCee/IDEA/compact/README.md and FCCee/ALLEGRO/compact/README.md, to say that you modified the beampipe to the CAD version

@aciarma
Copy link
Contributor Author

aciarma commented Jul 8, 2024

I put the IDEA_o1_v02/Beampipe_o4_v05.xml in the MDI_o1_v00 folder because it has the correct description of the central chamber, with AlBeMet and paraffin. I did not change the _v02 versions of the detectors anyway.

@BrieucF
Copy link
Contributor

BrieucF commented Jul 8, 2024

Thanks Andrea! @atolosadelgado is running one last test, shooting particle along the beampipe to see the CPU behavior in this case. Let's wait for his feedback

@BrieucF
Copy link
Contributor

BrieucF commented Jul 18, 2024

For the record:

  • running with the CAD based beampipe on ICP events from GuineaPig is 3x slower than with the shape based beampipe. Expected and ok given the level of details it has compared to the shape based
  • we decided to keep the shape based beampipe as default in the main detector xml, since only dedicated studies need it
  • having the CAD beampipe easily available is though a plus

The question becomes: is anyone objecting with the introduction of ~20MB CAD model in the git repository?

@BrieucF
Copy link
Contributor

BrieucF commented Jul 18, 2024

For the record:

* running with the CAD based beampipe on ICP events from GuineaPig is 3x slower than with the shape based beampipe. Expected and ok given the level of details it has compared to the shape based

* we decided to keep the shape based beampipe as default in the main detector xml, since only dedicated studies need it

* having the CAD beampipe easily available is though a plus

The question becomes: is anyone objecting with the introduction of ~20MB CAD model in the git repository?

Well, thinking twice I am a bit reluctant to add these files directly in the repository (would increase its size by 20% and we will have many different versions of these CAD files over time). Investigating other solutions...

@BrieucF
Copy link
Contributor

BrieucF commented Jul 18, 2024

Can you remove all the .stl files from this PR?

We will retrieve them from https://fccsw.web.cern.ch/fccsw/filesForSimDigiReco/MDI/MDI_o1_v01/. Since now the default is to use the shape based beampipe we could handle this in another PR.

Can you also fix the conflicts and update your branch?

@aciarma
Copy link
Contributor Author

aciarma commented Jul 18, 2024

I removed the STL files but left the MDI_o1_v01 folder, I have updated the README file to specify that the stl are not included for now

@BrieucF
Copy link
Contributor

BrieucF commented Jul 19, 2024

Thanks Andrea, can you resolve the conflicts now?

@BrieucF BrieucF changed the title CAD models for beampipe (and future MDI elements) [FCCee] Centralize beampipe geometries in an MDI folder Jul 19, 2024
@BrieucF
Copy link
Contributor

BrieucF commented Jul 19, 2024

The ALLEGRO_o1_v03 test is failing with Compact ERROR ++ FAILED to convert subdetector: BeBeampipe: dd4hep: Evaluator::Object : unknown variable : BPWWall : value=CentralBeamPipe_rmax+BPWWall [Evaluation error], can you fix?

@aciarma
Copy link
Contributor Author

aciarma commented Jul 19, 2024

test 13 - t_test_steeringFile is failing on the nightlies (CLIC compact file)

@BrieucF
Copy link
Contributor

BrieucF commented Jul 19, 2024

test 13 - t_test_steeringFile is failing on the nightlies (CLIC compact file)

Yes, this will be fixed soon AIDASoft/DD4hep#1293

@BrieucF BrieucF merged commit 245c056 into key4hep:main Jul 19, 2024
4 of 6 checks passed
@BrieucF
Copy link
Contributor

BrieucF commented Jul 19, 2024

Thanks Andrea!

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.

4 participants