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

move main ALLEGRO tests to ddsim+k4run #109

Merged

Conversation

giovannimarchiori
Copy link
Contributor

Move ALLEGRO tests to ddsim + k4run, and reduce number of tests

WIP: need to include in main test clustering with x-talk (and remove the last test FCCeeLar_runxtalk)

There are two tests (FCCeeLar_benchmarkCalibration and FCCeeLAr_benchmarkCorrection) for which I think that:

  • the former (FCCeeLar_benchmarkCalibration) is obsolete and could probably be dropped, and is basically superseded by the latter
  • the latter (FCCeeLAr_benchmarkCorrection) only works for ECAL barrel + HCAL barrel clusters, but the new ALLEGRO_o1_v03_digi_reco.py test script has CaloClusters assembled from all calorimeter systems, so we should decide whether the benchmark correction tool should be migrated to those clusters or whether for testing purposes only we create a collection with only barrel ECAL+HCAL cells included.

@giovannimarchiori giovannimarchiori changed the title WIP: move main ALLEGRO tests to ddsim+k4run move main ALLEGRO tests to ddsim+k4run Sep 2, 2024
@giovannimarchiori giovannimarchiori marked this pull request as ready for review September 2, 2024 15:15
@giovannimarchiori
Copy link
Contributor Author

Hi @BrieucF I think this PR is now ready for review, the new test supersedes a few tests and can then serve also as a template for run_digi_reco in FCC-config. When new tools are added (new cell digitisation + positioning tool, noise..) we can then create further PRs to update the test - or modify the test directly in the PRs implementing the new algorithms.

@BrieucF
Copy link
Contributor

BrieucF commented Sep 3, 2024

Hi Giovanni, it looks good! Thanks! The test should be modified whithin the PR adding a new functionality in k4RecCalorimeter ;-)

@BrieucF
Copy link
Contributor

BrieucF commented Sep 3, 2024

@kjvbrt is this ok for you?

@giovannimarchiori
Copy link
Contributor Author

Hi @kjvbrt, @BrieucF,
I pushed some small updates accounting for the comments of @kjvbrt. Once the tests are successful can we approve and merge?

Comment on lines +43 to +47
retcode=$?
if [ $retcode -ne 0 ]; then
echo "Simulation failed"
exit $retcode
fi
Copy link
Contributor

@kjvbrt kjvbrt Sep 4, 2024

Choose a reason for hiding this comment

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

It is easier to check for the non-zero return value with:

ddsim .... || exit 1

This will exit the script with 1 if ddsim returnns non zero value (the commands for or are evaluated from left to right and if the result of the first command is already true the second command is skipped).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will fix in next PR

@kjvbrt kjvbrt merged commit 389cfd9 into HEP-FCC:main Sep 4, 2024
2 of 5 checks passed
@giovannimarchiori giovannimarchiori deleted the gmarchio-main-20240829-moveteststoddsim branch September 4, 2024 13:14
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