Skip to content

[FIT] Separate Trigger class and make it common for FT0, FV0 and FDD.…#8543

Merged
shahor02 merged 2 commits intoAliceO2Group:devfrom
mslupeck:fitTrg
Apr 14, 2022
Merged

[FIT] Separate Trigger class and make it common for FT0, FV0 and FDD.…#8543
shahor02 merged 2 commits intoAliceO2Group:devfrom
mslupeck:fitTrg

Conversation

@mslupeck
Copy link
Collaborator

… Fix Trigger class initial values.

@mslupeck mslupeck force-pushed the fitTrg branch 2 times, most recently from 3f50d96 to cc40244 Compare April 11, 2022 08:12
Copy link
Contributor

@AllaMaevskaya AllaMaevskaya left a comment

Choose a reason for hiding this comment

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

I like your changes. The only thing I don't agree is
static const int16_t DEFAULT_TIME = -127; // for average/8 of one side (A or C)

  1. average time should not be divided by 8, only sum amplitude does;
  2. -127 is too sharp threshold, if we want vertex cut +-50cm we will be out of this. Time channels are from -2048 to +2048, so let set number out of this

@mslupeck
Copy link
Collaborator Author

mslupeck commented Apr 11, 2022

I like your changes. The only thing I don't agree is static const int16_t DEFAULT_TIME = -127; // for average/8 of one side (A or C)

  1. average time should not be divided by 8, only sum amplitude does;

Good catch - I modified the comment. Thanks!

  1. -127 is too sharp threshold, if we want vertex cut +-50cm we will be out of this. Time channels are from -2048 to +2048, so let set number out of this

Actually, I reverted it back to the previous value (-5000)

@mslupeck mslupeck marked this pull request as ready for review April 11, 2022 15:37
@mslupeck mslupeck requested review from a team, jotwinow and shahor02 as code owners April 11, 2022 15:37
@mslupeck mslupeck requested a review from AllaMaevskaya April 11, 2022 15:47
AllaMaevskaya
AllaMaevskaya previously approved these changes Apr 11, 2022
AllaMaevskaya
AllaMaevskaya previously approved these changes Apr 12, 2022
@mslupeck
Copy link
Collaborator Author

Hi @shahor02
Could you merge it?

Copy link
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

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

@mslupeck before I merge this: given the changes in the format and CTFCoder, did you check that the old, e.g. pilot beam data are readable with this PR?

@mslupeck
Copy link
Collaborator Author

@shahor02 - thanks for the hint. Now I've tested it using:
o2-ctf-reader-workflow --ctf-input o2_ctf_run00505582_orbit0316488698_tf0000167535.root --onlyDet FV0 | o2-fv0-digits-writer-workflow --disable-mc -b
o2-ctf-reader-workflow --ctf-input o2_ctf_run00505582_orbit0316488698_tf0000167535.root --onlyDet FT0 | o2-ft0-digits-writer-workflow --disable-mc -b
They were executed without errors and the output digit files are the same as in dev branch (except the sum of amplitude default value, which was modified from -5000 to 0 in this PR). I think it can be merged.

@shahor02 shahor02 merged commit 9cccd10 into AliceO2Group:dev Apr 14, 2022
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.

3 participants