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

version of ALLEGRO with ECAL barrel with cell corners projective in phi #332

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

giovannimarchiori
Copy link
Contributor

BEGINRELEASENOTES

  • New version of ALLEGRO detector o1_v03 with ecal barrel with 11 layers with cell corners projective along phi. No changes to the other sub detectors.
    ENDRELEASENOTES

@giovannimarchiori
Copy link
Contributor Author

Tagging @BrieucF

@giovannimarchiori
Copy link
Contributor Author

Hi @BrieucF thanks for having looked at the PR and spotted the little typo in the documentation. Is there anything else needed before merging?

Thanks, Giovanni

@BrieucF
Copy link
Contributor

BrieucF commented Apr 5, 2024

Hi Giovanni, yes sorry, I started to review then had to do something else. Can you add a test for this new ALLEGRO version? https://github.com/key4hep/k4geo/blob/main/lcgeoTests/CMakeLists.txt#L90-L94

@giovannimarchiori
Copy link
Contributor Author

Hi Giovanni, yes sorry, I started to review then had to do something else. Can you add a test for this new ALLEGRO version? https://github.com/key4hep/k4geo/blob/main/lcgeoTests/CMakeLists.txt#L90-L94

done

@giovannimarchiori
Copy link
Contributor Author

downstream-build errors are unrelated

The following tests FAILED:
2 - t_test_CLIC_o3_v14_reco_conformal (SEGFAULT)
3 - t_test_CLIC_o3_v14_vtx_makentp (Failed)
5 - t_test_FCCee_o1_v04_reco_conformal (Subprocess aborted)

@giovannimarchiori
Copy link
Contributor Author

I just added some comments to the C++ code that builds the LAr detector to make it a bit easier to follow the steps of the G4 construction, and some comments to the corresponding xml file to clarify the meaning of the vector with the layer thicknesses

@BrieucF
Copy link
Contributor

BrieucF commented Apr 11, 2024

Can you also use the v03 constructor for upstream and calibration xml's?

@giovannimarchiori
Copy link
Contributor Author

Yes, when we have agreed on the rounding ;)

@BrieucF
Copy link
Contributor

BrieucF commented Apr 11, 2024

Ha sorry, missed the last mail, taking a look

…ments in xml of v02. In cpp of v03 add check that electrode length in xml is consistent with calo r1 r2 and inclination angle
@giovannimarchiori
Copy link
Contributor Author

@BrieucF : I updated also the other xml files for v03. In the xml of v02 I clarified that the dimensions are electrode lengths scaled by calo dR/L(plane). In the cpp of v03 I also added a check that the electrode plane length in the xml is consistent within 0.5 mm with that calculated from calo R1, R2 and alpha

Let me check with Nicolas if he's OK with these non rounded numbers and then I think we're good to merge

@BrieucF
Copy link
Contributor

BrieucF commented Apr 12, 2024

Thanks Giovanni! LGTM!

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.

One should also use units in the CPP implementation

@giovannimarchiori
Copy link
Contributor Author

Fixed in latest commit

@giovannimarchiori
Copy link
Contributor Author

Hello @andresailer,
please let me know if I need to do further modifications or if the code can be merged as it is now.
Cheers,
Giovanni

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.

Nothing else from my side.
Thanks!

@BrieucF
Copy link
Contributor

BrieucF commented Apr 16, 2024

Thanks Giovanni and Andre!

@BrieucF BrieucF merged commit 1e73911 into key4hep:main Apr 16, 2024
8 of 9 checks passed
@giovannimarchiori giovannimarchiori deleted the gmarchio-main-20240327 branch July 22, 2024 12:42
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.

3 participants