Skip to content

Conversation

@KangkanGoswami
Copy link

This change contains the extra tables for TRD PID Study.

@KangkanGoswami KangkanGoswami requested review from a team as code owners September 22, 2025 11:40
@github-actions
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1
async-2024-PbPb-apass2
async-2023-PbPb-apass5

@wille10 wille10 self-requested a review September 24, 2025 10:37
wille10
wille10 previously approved these changes Sep 24, 2025
@alibuild
Copy link
Collaborator

Error while checking build/O2/fullCI_slc9 for 47a07e7 at 2025-09-24 14:16:

## sw/BUILD/O2-latest/log
/sw/SOURCES/O2/14683-slc9_x86-64/0/Detectors/AOD/src/AODProducerWorkflowSpec.cxx:1991:47: error: redeclaration of 'o2::framework::Produces<o2::soa::Table<o2::aod::Hash<4125400158>, o2::aod::Hash<3317288390>, o2::aod::Hash<2286545062> > > ft0ExtraCursor'
/sw/SOURCES/O2/14683-slc9_x86-64/0/Detectors/AOD/src/AODProducerWorkflowSpec.cxx:1992:47: error: redeclaration of 'o2::framework::Produces<o2::soa::Table<o2::aod::Hash<2440583501>, o2::aod::Hash<1097011421>, o2::aod::Hash<2286545062> > > fddExtraCursor'
/sw/SOURCES/O2/14683-slc9_x86-64/0/Detectors/AOD/src/AODProducerWorkflowSpec.cxx:1993:48: error: redeclaration of 'o2::framework::Produces<o2::soa::Table<o2::aod::Hash<3518865614>, o2::aod::Hash<898762686>, o2::aod::Hash<2286545062> > > fv0aExtraCursor'
ninja: build stopped: subcommand failed.

Full log here.

I have now removed ft0ExtraCursor, fddExtraCursor, and fv0aExtraCursor. This should, in principle, resolve the issue of re-definition.
@f3sch
Copy link
Collaborator

f3sch commented Sep 26, 2025

@jgrosseo, could you please approve for the missing security check, so at least the CI can run, thanks!

Copy link
Collaborator

@sawenzel sawenzel left a comment

Choose a reason for hiding this comment

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

This is a major addition to the AOD writer and data model. To continue here, I think this should first of all be presented and discussed in the Analysis meeting (if it hasn't happened yet) or be supported by Physics Coordination. It would also be good to know if this is supposed to be included by default or only in very specific studies.

For now a few comments from my side are:

(a) please try to give a more comprehensive message to the PR. Just saying "This is for the TRD PID study" is a bit too short. Good common practice is "This PR provides new (optional) tables in the AOD that are needed for...; This has been discussed in this JIRA ticket or in these slides"

(b) please try to cleanup this PR. It should only be 1 clean commit

template <typename TRDsExtraCursorType>
void AODProducerWorkflowDPL::addToTRDsExtra(const o2::globaltracking::RecoContainer& recoData, TRDsExtraCursorType& trdExtraCursor, const GIndex& trkIdx, int trkTableIdx)
{
static int q0s[6] = {-1}, q1s[6] = {-1}, q2s[6] = {-1};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a lot of new detector specific code. Could this not be done in TRD reco for better logic isolation? Also, I would be interested to know the CPU impact on the overall AOD processing.

@KangkanGoswami
Copy link
Author

Hi @sawenzel, my apologies for such a messy PR. I presented these changes in the weekly TRD meeting. To clean up this PR, I would close the current one and create a new PR as soon as possible.

@sawenzel
Copy link
Collaborator

Hi @sawenzel, my apologies for such a messy PR. I presented these changes in the weekly TRD meeting. To clean up this PR, I would close the current one and create a new PR as soon as possible.

Thanks but you don't need to close it. You can just squash locally + make a good commit message and force-push to the same PR.

@alibuild
Copy link
Collaborator

alibuild commented Oct 8, 2025

Error while checking build/O2/fullCI_slc9 for 111e983 at 2025-10-08 05:51:

No log files found

Full log here.

@KangkanGoswami
Copy link
Author

This commit requires some cleanup and a clearer description. Due to this, I am closing this commit and starting a new one with a few modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants