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

FIT/FT0: new functions for FT0 to reconstruct event plane #11651

Closed
wants to merge 3 commits into from

Conversation

jikim1290
Copy link
Contributor

new functions for FT0 to reconstruct event plane

jikim1290 added a commit to jikim1290/AliceO2 that referenced this pull request Jul 12, 2023
Please consider the following formatting changes to AliceO2Group#11651
Comment on lines 176 to 188
Int_t localChannelOrder[Nchannels] = {
58, 56, 59, 57, 54, 52, 55, 53, 50, 49, 51, 48, 47, 45, 46, 44, 43, 42, 41, 40,
61, 60, 63, 62, 14, 12, 15, 13, 10, 9, 11, 8, 7, 6, 5, 4, 39, 38, 37, 36,
65, 64, 66, 67, 17, 16, 18, 19, 3, 2, 0, 1, 35, 34, 32, 33,
68, 69, 70, 71, 20, 21, 22, 23, 24, 27, 25, 26, 29, 31, 28, 30, 94, 95, 92, 93,
72, 73, 74, 75, 76, 78, 77, 79, 80, 83, 81, 82, 85, 87, 84, 86, 89, 91, 88, 90,
173, 172, 175, 174, 206, 207, 204, 205, 169, 168, 171, 170, 202, 203, 200, 201,
117, 116, 119, 118, 142, 143, 140, 141, 114, 112, 115, 113, 137, 139, 136, 138,
166, 164, 167, 165, 197, 199, 196, 198, 110, 108, 111, 109, 133, 135, 132, 134,
162, 160, 163, 161, 193, 195, 192, 194, 107, 105, 106, 104, 128, 130, 129, 131,
159, 157, 158, 156, 188, 190, 189, 191, 99, 98, 97, 96, 120, 121, 122, 123,
103, 102, 101, 100, 124, 125, 126, 127, 155, 153, 154, 152, 184, 186, 185, 187,
147, 146, 145, 144, 176, 177, 178, 179, 151, 150, 149, 148, 180, 181, 182, 183};
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for my curiosity , is it possible to make such parameters configurable(via key-value config or as CCDB object, etc)? Or they should be always compile-time/hardcoded?

Copy link
Contributor

Choose a reason for hiding this comment

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

one could also put such a mapping in the CCDB of course. But should I merge this PR as is in any case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your idea for taking information from CCDB. However, this is for coming Pb-Pb data and the current analyzers are suffering from very slow CCDB connection. It can be surely improved later.

Copy link
Collaborator

@afurs afurs Jul 14, 2023

Choose a reason for hiding this comment

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

But such CCDB request would be only once at the init stage or may be cached into file and then used several times(e.g. for "multinode" simulation mode, similar we have in anchored MC as far as I remember). Would it be still very slow in your case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

just for my curiosity , is it possible to make such parameters configurable(via key-value config or as CCDB object, etc)? Or they should be always compile-time/hardcoded?

@afurs for the record: configurable key-value supports C-type arrays.

// Calculate the coordinates of all the channels.
void calculateChannelCenter();
// Get the coordinates of the center of the channel channelId.
TVector3 getChannelCenter(UInt_t channelId) { return mChannelCenter[channelId]; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

please put const

Copy link
Contributor

Choose a reason for hiding this comment

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

since this returns a copy in any case I don't think const makes a difference, but up to you of course the way you want to use this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added const for integrity of the system.

jikim1290 added a commit to jikim1290/AliceO2 that referenced this pull request Jul 12, 2023
Please consider the following formatting changes to AliceO2Group#11651
afurs
afurs previously approved these changes Jul 12, 2023
@alibuild
Copy link
Collaborator

Error while checking build/O2/fullCI for b7793e5 at 2023-07-12 20:54:

No log files found

Full log here.

jikim1290 added a commit to jikim1290/AliceO2 that referenced this pull request Jul 14, 2023
Please consider the following formatting changes to AliceO2Group#11651
afurs
afurs previously approved these changes Jul 14, 2023
@jikim1290
Copy link
Contributor Author

The test fails and said:

sw/BUILD/QualityControl-latest/log

1/31 Test #1: o2-qc-test-core .........................***Timeout 30.05 sec
97% tests passed, 1 tests failed out of 31

Is it due to problem from my side? I don't see anything wrong..

162, 160, 163, 161, 193, 195, 192, 194, 107, 105, 106, 104, 128, 130, 129, 131,
159, 157, 158, 156, 188, 190, 189, 191, 99, 98, 97, 96, 120, 121, 122, 123,
103, 102, 101, 100, 124, 125, 126, 127, 155, 153, 154, 152, 184, 186, 185, 187,
147, 146, 145, 144, 176, 177, 178, 179, 151, 150, 149, 148, 180, 181, 182, 183};
ClassDefNV(Geometry, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, when adding data members you should increase also the ClassDef version. The CI error is unrelated to your changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I have corrected.

@martenole
Copy link
Contributor

Hm but now something seems wrong since you only changed the ClassDef version, nothing more. Did a commit get lost?

@jikim1290
Copy link
Contributor Author

Hm but now something seems wrong since you only changed the ClassDef version, nothing more. Did a commit get lost?

I restored my update, Thank you for your nice notice

@jikim1290
Copy link
Contributor Author

jikim1290 commented Jul 20, 2023

Can you please give an approval to test?

Copy link
Contributor

@martenole martenole left a comment

Choose a reason for hiding this comment

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

Approving for CI to start

@jikim1290 jikim1290 closed this Jul 24, 2023
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.

5 participants