Skip to content

[Common] TrackQAConverter002- for TOF modification #8901

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.

Sign up for GitHub

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

Jump to bottom
Merged
pzhristov merged 1 commit into from
Dec 12, 2024
Merged

[Common] TrackQAConverter002- for TOF modification #8901

pzhristov merged 1 commit into from
Dec 12, 2024

Conversation

@miranov25
Copy link
Contributor

@miranov25 miranov25 commented Dec 10, 2024

Relates to the O2 modification - AliceO2Group/AliceO2#13787

@f3sch
Common: TrackQAConverter002
9fcdd15 
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
@github-actions github-actions bot changed the title Common: TrackQAConverter002- for TOF modification [Common] TrackQAConverter002- for TOF modification Dec 10, 2024
@miranov25
Copy link
Contributor Author

miranov25 commented Dec 10, 2024

Code modification provided by @f3sch. It will need O2 modifications merged before.

@alibuild
Copy link
Collaborator

alibuild commented Dec 10, 2024 •
edited
Loading

Error while checking build/O2Physics/o2 for 9fcdd15 at 2024-12-12 04:35:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/8901-slc9_x86-64/0/Common/TableProducer/Converters/trackQA002Converter.cxx:21:17: error: 'TracksQA_002' is not a member of 'o2::aod'; did you mean 'TracksQA_001'?
/sw/SOURCES/O2Physics/8901-slc9_x86-64/0/Common/TableProducer/Converters/trackQA002Converter.cxx:21:29: error: template argument 1 is invalid
/sw/SOURCES/O2Physics/8901-slc9_x86-64/0/Common/TableProducer/Converters/trackQA002Converter.cxx:25:32: error: 'tracksQA_000' was not declared in this scope; did you mean 'tracksQA_002'?
/sw/SOURCES/O2Physics/8901-slc9_x86-64/0/Common/TableProducer/Converters/trackQA002Converter.cxx:59:32: error: 'tracksQA_001' was not declared in this scope; did you mean 'tracksQA_002'?
ninja: build stopped: subcommand failed.

Full log here.

@miranov25 miranov25 mentioned this pull request Dec 11, 2024
Merged
@pzhristov pzhristov merged commit a8d5ca2 into AliceO2Group:master Dec 12, 2024
8 of 12 checks passed
@miranov25
Copy link
Contributor Author

miranov25 commented Dec 12, 2024

Hello @vkucera and @pzhristov,

This pull request requires the O2 repository to be updated. The O2 update was merged a few minutes ago.

In my case, everything works well once both repositories are synchronized. What is the usual strategy for syncing O2 and O2Physics? @f3sch mentioned that we might need to wait for some time.

Best regards,
Marian

@ktf
Copy link
Member

ktf commented Dec 12, 2024

In general we first merge in O2, do a build and then merge the O2Physics changes. @singiamtel can you trigger a new nightly to fix this? Thanks.

@singiamtel
Copy link
Contributor

singiamtel commented Dec 12, 2024

New nightly build ongoing at https://alijenkins.cern.ch/job/DailyBuilds/job/DailyO2Physics-slc9/349/

vkucera
vkucera reviewed Dec 12, 2024
View reviewed changes
Common/TableProducer/Converters/trackQA002Converter.cxx
Comment on lines +21 to +26
Produces<aod::TracksQA_002> tracksQA_002;

void process000(aod::TracksQA_000 const& tracksQA_002)
{
for (const auto& trackQA : tracksQA_000) {
tracksQA_002(
Copy link
Collaborator

@vkucera vkucera Dec 12, 2024

Choose a reason for hiding this comment

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

Sorry, but this is clearly a bug which does not compile.

Copy link
Contributor Author

@miranov25 miranov25 Dec 12, 2024

Choose a reason for hiding this comment

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

Thank you for checking. I got the modification from Felix, and I assume that I compiled it.
Looks like it was not synced to my build server properly

vkucera
vkucera reviewed Dec 12, 2024
View reviewed changes
Common/TableProducer/Converters/trackQA002Converter.cxx
Comment on lines +57 to +60
void process001(aod::TracksQA_001 const& tracksQA_002)
{
for (const auto& trackQA : tracksQA_001) {
tracksQA_002(
Copy link
Collaborator

@vkucera vkucera Dec 12, 2024

Choose a reason for hiding this comment

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

Same here

@miranov25
Copy link
Contributor Author

miranov25 commented Dec 12, 2024

Hello @vkucera
I see. I commented above.

Thank you for checking. I got the modification from Felix, and I assume that I compiled it.
Looks like it was not synced to my build server properly.

Please wait for change

miranov25 pushed a commit to miranov25/O2Physics that referenced this pull request Dec 12, 2024
Fix the bug -PR8901
833e632 
AliceO2Group#8901 (comment)
@miranov25
Copy link
Contributor Author

miranov25 commented Dec 12, 2024

Hello @vkucera,

The code has compiled successfully with the fix applied above. It will take another 15 minutes to complete the full compilation (converter was already compiled).

Since I haven’t tested the converter before, could you advise on the recommended way to test it?

I typically work with AO2D files in ROOT sessions and Python, using derived data sets, but I’m not sure how to properly validate the converter.

Best regards,
Marian

@vkucera
Copy link
Collaborator

vkucera commented Dec 12, 2024

Hi @miranov25 , thanks, I confirm that O2Physics compiles with the fix above. Please make a PR.

@miranov25
Copy link
Contributor Author

miranov25 commented Dec 12, 2024 •
edited
Loading

Can I reuse this pull request, or should I create new one? As in the description it was written the pull request was already merged, i am not sure what to do.

@vkucera
Copy link
Collaborator

vkucera commented Dec 12, 2024

@miranov25 I don't think you have to do anything complicated to validate the converter. It should be enough to run the workflow on a file with TracksQA_000 and on a file with TracksQA_001 and check the output tables.

@vkucera
Copy link
Collaborator

vkucera commented Dec 12, 2024

Can I reuse this pull request, or should I create new one?

This PR has been merged. You have to make a new one, although you can use the same branch.

miranov25 pushed a commit to miranov25/O2Physics that referenced this pull request Dec 12, 2024
Fix the bug -PR8901
77e6d4b 
AliceO2Group#8901 (comment)
@miranov25 miranov25 mentioned this pull request Dec 12, 2024
Merged
@miranov25
Copy link
Contributor Author

miranov25 commented Dec 12, 2024

Creating pull request with fix - #8961

@vkucera
Copy link
Collaborator

vkucera commented Dec 12, 2024

Creating pull request with fix - #8961

Thanks

@ddobrigk
Copy link
Collaborator

ddobrigk commented Dec 12, 2024 •
edited
Loading

It would make sense to click revert and force-merge the revert - could we do that, @pzhristov ? We anyway cannot merge the fix right now...

ddobrigk added a commit that referenced this pull request Dec 12, 2024
@ddobrigk
Revert "[Common] TrackQAConverter002- for TOF modification (#8901)"
6a6e182 
This reverts commit a8d5ca2.
@ddobrigk ddobrigk mentioned this pull request Dec 12, 2024
Closed
@singiamtel
Copy link
Contributor

singiamtel commented Dec 12, 2024

In general we first merge in O2, do a build and then merge the O2Physics changes. @singiamtel can you trigger a new nightly to fix this? Thanks.

Restarting the nightly as the fix was merged

hernasab pushed a commit to hernasab/O2Physics that referenced this pull request Dec 20, 2024
@miranov25 @f3sch
[Common] TrackQAConverter002- for TOF modification (AliceO2Group#8901)
48037ac 
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
Co-authored-by: Felix Schlepper <felix.schlepper@cern.ch>
feisenhu pushed a commit to feisenhu/O2Physics that referenced this pull request Jan 8, 2025
@miranov25 @f3sch @feisenhu
[Common] TrackQAConverter002- for TOF modification (AliceO2Group#8901)
4625f83 
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
Co-authored-by: Felix Schlepper <felix.schlepper@cern.ch>
smaff92 pushed a commit to smaff92/O2Physics that referenced this pull request Feb 17, 2025
@miranov25 @f3sch @smaff92
[Common] TrackQAConverter002- for TOF modification (AliceO2Group#8901)
ed7e39d 
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
Co-authored-by: Felix Schlepper <felix.schlepper@cern.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vkucera vkucera vkucera left review comments

@alibuild alibuild Awaiting requested review from alibuild alibuild is a code owner

@jgrosseo jgrosseo Awaiting requested review from jgrosseo jgrosseo is a code owner

@iarsene iarsene Awaiting requested review from iarsene iarsene is a code owner

@ktf ktf Awaiting requested review from ktf ktf is a code owner

@ddobrigk ddobrigk Awaiting requested review from ddobrigk ddobrigk is a code owner

Assignees

No one assigned

Labels

common

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@miranov25 @alibuild @ktf @singiamtel @vkucera @ddobrigk @pzhristov @f3sch