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

(QVecDev) Added tables with new labeling #7199

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

MaximVirta
Copy link
Contributor

New labeling added for the Q-vector tables, with backwards compatibility with the old labeling convetion.
Internal variables have been changed directly.

Comment on lines +46 to +51
DECLARE_SOA_COLUMN(QvecTPCposReVec, qvecTPCposReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCposImVec, qvecTPCposImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCnegReVec, qvecTPCnegReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCnegImVec, qvecTPCnegImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCallReVec, qvecTPCallReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCallImVec, qvecTPCallImVec, std::vector<float>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use proper camelCase for good readability. Qvec, Cpos, Cneg, Call are not words nor abbreviations.

Suggested change
DECLARE_SOA_COLUMN(QvecTPCposReVec, qvecTPCposReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCposImVec, qvecTPCposImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCnegReVec, qvecTPCnegReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCnegImVec, qvecTPCnegImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCallReVec, qvecTPCallReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QvecTPCallImVec, qvecTPCallImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QVecTpcPosReVec, qVecTpcPosReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QVecTpcPosImVec, qVecTpcPosImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QVecTpcNegReVec, qVecTpcNegReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QVecTpcNegImVec, qVecTpcNegImVec, std::vector<float>);
DECLARE_SOA_COLUMN(QVecTpcAllReVec, qVecTpcAllReVec, std::vector<float>);
DECLARE_SOA_COLUMN(QVecTpcAllImVec, qVecTpcAllImVec, std::vector<float>);

This applies to all other name changes too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the "Qvec" -> "QVec" part, I would prefer not to change those as there are many tasks which are already dependent on these labeling and would need to be changed as well. The "TPCpos" -> "TpcPos" I can change, but why should we change only TPC to be in lower case, when other detectors are in capitals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would "TPCPos" be a better option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your PR itself is titled QVecDev, includes files qVectorsTable.cxx, qVectorsCorrection.cxx, and defines variables like qVectorBPosVec. So apparently the capitalisation of v is very common in your code and not an issue. So I would say that the occurrences of lower-case v (e.g. Qvectors.h) should be fixed to avoid inconsistencies.

Copy link
Collaborator

@vkucera vkucera Aug 8, 2024

Choose a reason for hiding this comment

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

Concerning the capitalisation of TPC, while it might be fine at the beginning of a word, it becomes less readable when in the middle of the word and very unreadable when combined with another detector acronym, like TPCTOF. See the examples ITSReco vs numberOfDnsConnections in the naming conventions.
Other detectors are not exceptions, of course.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So TPCPos is definitely better than TPCpos but TpcPos is even better.

Copy link
Collaborator

@strogolo strogolo Aug 8, 2024

Choose a reason for hiding this comment

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

Dear @vkucera,
we prefer not to change the capitalisation of the v because for the other detectors used in the Q-vector calculation this is the convention. So it would create troubles and differences.

Please @MaximVirta keep the v not capital and be consistent with the rest of the Q-vector framework.
This PR, as discussed with @ddobrigk, is intended to fix a renaming issue and not to change the convention adopted by us.

@ddobrigk ddobrigk merged commit 2eaf25e into AliceO2Group:master Aug 8, 2024
17 checks passed
@ddobrigk
Copy link
Collaborator

ddobrigk commented Aug 8, 2024

Thank you @MaximVirta !

Luca610 pushed a commit to Luca610/O2Physics that referenced this pull request Aug 13, 2024
* (QVecDev) Added tables with new labeling

* Formatting fixed
joonsukbae pushed a commit to joonsukbae/O2Physics that referenced this pull request Aug 27, 2024
* (QVecDev) Added tables with new labeling

* Formatting fixed
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.

4 participants