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

refactor TRD digit class #5221

Merged
merged 1 commit into from Jan 19, 2021
Merged

refactor TRD digit class #5221

merged 1 commit into from Jan 19, 2021

Conversation

bazinski
Copy link
Collaborator

  • Convert digits to store rob/mcm and not pad/row.
  • The interface for pad row remains so code wont change.
  • For shared pads the lower indexed mcm is returned. It is the users responsibility to check if its shared if needed for their purposes.
  • The plan was to move digit to DataFormats, but there is a circular library, so postponed to later.

Copy link
Collaborator

@tdietel tdietel left a comment

Choose a reason for hiding this comment

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

I think the indicator for the shared pads is buggy and should be fixed.

Detectors/TRD/base/include/TRDBase/Digit.h Outdated Show resolved Hide resolved
Detectors/TRD/base/src/FeeParam.cxx Show resolved Hide resolved
Detectors/TRD/simulation/src/Digitizer.cxx Show resolved Hide resolved
Copy link
Collaborator

@jolopezl jolopezl left a comment

Choose a reason for hiding this comment

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

I am happy with the Digit class changes.

Just a minor style comment: I prefer to read ROB instead of Rob, as in the Tracklet class (mROB, getROB, setROB), to make the code more consistent and easier to browse.

Detectors/TRD/base/include/TRDBase/Digit.h Outdated Show resolved Hide resolved
Detectors/TRD/base/include/TRDBase/Digit.h Outdated Show resolved Hide resolved
Detectors/TRD/simulation/src/Digitizer.cxx Show resolved Hide resolved
@bazinski bazinski force-pushed the changedigits branch 2 times, most recently from 8996a79 to d51dbeb Compare January 18, 2021 23:13
Detectors/TRD/base/include/TRDBase/Digit.h Outdated Show resolved Hide resolved
Detectors/TRD/base/src/FeeParam.cxx Outdated Show resolved Hide resolved
@bazinski
Copy link
Collaborator Author

@shahor02 shot for seeing, now fixed.

void setROB(int row, int pad) { mROB = FeeParam::getROBfromPad(row, pad); }
void setMCM(int mcm) { mMCM = mcm; }
void setMCM(int row, int pad) { mMCM = FeeParam::getMCMfromPad(row, pad); }
void setChannel(int channel) { mChannel = channel; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these setters actually used? I would expect that we always know at construction which pad these digits refer to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer keeping them for completeness; they are harmless.

tdietel
tdietel previously approved these changes Jan 19, 2021
Copy link
Collaborator

@tdietel tdietel left a comment

Choose a reason for hiding this comment

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

I added one or two more comments, but I think we can commit as it is.

@bazinski
Copy link
Collaborator Author

@tdietel let me change the rob value to uint8_t that will have somewhat of an impact on the sizing of the output, assuming it does not get padded.

@bazinski
Copy link
Collaborator Author

@tdietel regarding the set methods, i dont use them, they are simply there for completeness.

@shahor02
Copy link
Collaborator

@tdietel let me change the rob value to uint8_t that will have somewhat of an impact on the sizing of the output, assuming it does not get padded.

It will be padded

@bazinski
Copy link
Collaborator Author

@shahor02 when you get a chance could you approve the change as per your request and merge?

@shahor02 shahor02 merged commit 47fcf7b into AliceO2Group:dev Jan 19, 2021
@bazinski bazinski deleted the changedigits branch January 19, 2021 15:38
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.

None yet

4 participants