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

Fix for incorrect columns order in FwdTracksCov table #13751

Closed
wants to merge 1 commit into from

Conversation

Elros60
Copy link
Contributor

@Elros60 Elros60 commented Nov 28, 2024

No description provided.

@Elros60 Elros60 requested a review from a team as a code owner November 28, 2024 15:08
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-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1

@jgrosseo
Copy link
Collaborator

Where does this matter?

@Elros60
Copy link
Contributor Author

Elros60 commented Nov 28, 2024

Where does this matter?
As we see here, at the moment we fill the table in AODProducerWorkflowSpec, it followed the correct order fwdCovInfo.rhoPhiX first then fwdCovInfo.rhoPhiY; but in table definition, this order was inverted.
Screenshot 2024-11-28 at 16 31 11

@jgrosseo
Copy link
Collaborator

This means that all current data has the values inverted. And by this PR, so changing it in the definition, we will not solve it for old data. Is this acceptable?

If one wants to recover old data, one would need to

  • make a version 1 of the table which has the order fixed as you say
  • make a converter which swaps the two values

I cc also @ddobrigk for his opinion.

@jgrosseo jgrosseo marked this pull request as draft November 28, 2024 16:21
@jgrosseo
Copy link
Collaborator

I put it to draft for now until the discussion is finished.

@Elros60
Copy link
Contributor Author

Elros60 commented Nov 28, 2024

This means that all current data has the values inverted. And by this PR, so changing it in the definition, we will not solve it for old data. Is this acceptable?

If one wants to recover old data, one would need to

  • make a version 1 of the table which has the order fixed as you say
  • make a converter which swaps the two values

I cc also @ddobrigk for his opinion.

I would cc here Luca @lucamicheletti93, Maurice @MauriceCoke and Ionut @iarsene here for opinions from PWGDQ.

@ddobrigk
Copy link
Contributor

ddobrigk commented Nov 28, 2024

I agree with @jgrosseo's concern... if it turns out that this is a silent fix, it might lead to a lot of potential problems, and the best is to use versioning to switch the elements. We had a similar problem with PV covariance elements in the past and we also followed that approach. The only downside is the necessity of a converter... But it's perhaps better to be safe, as it would be very very difficult, in practice, to keep track of instances in which swapped data was used.

Maybe one more question: in typical FwdTrack analyses, is the covariance very relevant / used? Just for me to understand the consequence of the fix in general. Thanks a lot!!

@iarsene
Copy link
Collaborator

iarsene commented Nov 29, 2024

@ddobrigk Covariance is indeed used in forward tracks since we have to do the matching with MFT and then further with the primary vertex and also reconstructing secondary vertices. Just as for the barrel.

So, as @jgrosseo, I would also think that we should fix this properly and implement a converter for the old data. Even if we fix this at the level of DQ where these tables are mostly used, we would have to use a converter anyway. So we need to do it in the O2 data model.

Copy link
Contributor

This PR did not have any update in the last 30 days. Is it still needed? Unless further action in will be closed in 5 days.

@github-actions github-actions bot added the stale label Dec 30, 2024
@github-actions github-actions bot closed this Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants