-
Notifications
You must be signed in to change notification settings - Fork 418
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
IT3: Full simulation + Glo-tracking #13069
base: dev
Are you sure you want to change the base?
Conversation
REQUEST FOR PRODUCTION RELEASES:
This will add The following labels are available |
Hi @f3sch, I have a couple of more general comments/questions:
|
Hi @mconcas, Thanks for the feedback!
Agreed.
One example for this would be in the digit branch names where the detID is stringified. Also, I have no preference on how the flags for the workflows are named, so I will change them.
I do not mind implementing this switch but what would be the use-case of having it, since ITS and ITS3 are mutually exclusive so there will not be any overlap in the first place?
Naming things is hard.... and yes, your suggestions makes more sense, will change. |
33316b4
to
8f55fbc
Compare
Error while checking build/O2/fullCI for 8f55fbc at 2024-05-08 18:32:
Full log here. |
Failures seem unrelated. @sawenzel When you have time, could you please take a look at the proposed changes for |
@ktf When you have time, would you please take a look at the proposed CMake changes? Thanks. |
Why don't you use an expression for the upgrade libraries? I do not like the idea of embedding too much extensions to o2_add_library. In general that's a recipe for disasters. |
I.e. |
@f3sch @mconcas . In general nice developments and big progress! But I have a few comments to this PR. (a) just f.y.i; as far as I remember the limit of 3 letters for DetID strings does not come from FairRoot! I believe this is more part of the DPL (header) protocol. (b) I am not a big fan of adding new specific options to
(c) Along the same line, is it really necessary to modify multiple function signatures with new arguments |
Thanks @ktf, dropped the cmake changes and replaced them by expressions. |
@sawenzel thanks for the comments, dropped the sim changes in favour of the other PR and indeed using the configKeyValues is better. |
Hi @wille10, the changes for TRD should be minimal. I added a configKeyValue as for TRD there wasn't one before, for now this only holds a single value but maybe in the future can be extended. Do the changes seem fine to you? |
For the TRD reconstruction the parameters are stored in |
@martenole sorry I pinged the wrong person. I would prefer to have the settings consistent across the workflows, e.g., using parameters and not wf options. Correct me if I am wrong, the GPUSettings seem to be loaded after the request for the ITS clusters has been made, for ITS3 we need to conditionally load a different topology dictionary hence I need it to the input spec. |
Hm, ok. The GPU settings are indeed not the place for that setting. And the configurable parameter classes that we have (for calibration and simulation) should not be abused for this. Then I think it is OK to add the reconstruction class. And no worries, @wille10 is the correct person to tag for TRD now, but I also saw it so thought I can as well reply :) |
@f3sch sorry if I missed something, but why not defining a single key |
Thanks for taking the time :) |
Hi @shahor02, indeed it might be nicer to have it more as a global option instead of scattering this about in different files. I took a look but only found |
@f3sch then you can add a new ConfigurableParam, e.g. GlobalParams in the Detectors/Base, could be useful also for the future additions. |
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
@shahor02 thanks, removed all other additions to configparams in favour of newly added GlobalParms. |
Error while checking build/O2/fullCI for 4a58432 at 2024-05-24 17:11:
Full log here. |
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
Error while checking build/O2/fullCI for b2f8b88 at 2024-05-27 23:07:
Full log here. |
This PR aims to incorporate IT3 into the global-reconstruction chain + make simulation a bit easier:
The flags and functionality is hidden behind the compile flag 'ENABLE_UPGRADES' + the default is off.
I tested the full simulation chain for ENABLE_UPGRADES=On/Off + w. & w/o. IT3.
I can also make different PRs (however reviewers want :)). A brief overview: