Add good event flag to ETOF headers and StPicoEvent #160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…l setters from etofIn001 version of this file. Needed to unpack Get4Status bit into event files
…librations in the repository version. Nessecary only for creating calibrations, but should be available in the offical code. Added calculation of goodEventFlag for every counter from Get4 missmatch bits and pulser digis.
…librations in the repository version. Nessecary only for creating calibrations, but should be available in the offical code. Added calculation of goodEventFlag for every counter from Get4 missmatch bits and pulser digis.
PhilippWeidenkaff
requested review from
klendathu2k,
marrbnl,
nigmatkulov and
plexoos
as code owners
ullrich-bnl
approved these changes
Oct 4, 2021
veprbl
reviewed
Oct 5, 2021
| @@ -349,7 +349,7 @@ class StPicoDstMaker : public StMaker { | |||
| return cvs; | |||
| } | |||
|
|
|||
| ClassDef(StPicoDstMaker, 0) | |||
| ClassDef(StPicoDstMaker, 1) | |||
There was a problem hiding this comment.
Should stay at zero unless you plan to use streamers
There was a problem hiding this comment.
fixed in latest commit
marrbnl
reviewed
Oct 7, 2021
plexoos
reviewed
Oct 11, 2021
Comment on lines
+85
to
+99
| StETofHeader::StETofHeader( const double& trgGdpbTime, const double& trgStarTime, | ||
| const map< unsigned int, uint64_t >& gdpbTs, const map< unsigned int, uint64_t >& starTs, | ||
| const unsigned int& starToken, const unsigned int& starDaqCmdIn, const unsigned int& starTrgCmdIn, | ||
| const uint64_t& eventStatusFlag, const std::vector<bool>& MissMatchFlagVec, const std::vector<bool>& GoodEventFlagVec ) | ||
| : mTrgGdpbFullTime( trgGdpbTime ), | ||
| mTrgStarFullTime( trgStarTime ), | ||
| mStarToken( starToken ), | ||
| mStarDaqCmdIn( starDaqCmdIn ), | ||
| mStarTrgCmdIn( starTrgCmdIn ), | ||
| mEventStatusFlag( eventStatusFlag ), | ||
| mMissMatchFlagVec( MissMatchFlagVec ), | ||
| mGoodEventFlagVec( GoodEventFlagVec ) | ||
| { | ||
| setRocGdpbTs( gdpbTs ); | ||
| setRocStarTs( starTs ); |
There was a problem hiding this comment.
You could use delegating constructors to reduce all of this boireplate code for StETofHeader constructors.
There was a problem hiding this comment.
This may be a good suggestion, but IMO it leans into refactoring of the existing code. Would be nice to see it, but in a separate PR?
|
Hello Reviewer,
Sorry for the late reaction. Appearently something got messed up in my rebasing process and i'm still working on fixing this.The rebasing process is very slow (taking multiple days even if it is not crashing). I will write you when i upload a new version with all the requested fixes.
Best regards
Philipp Weidenkaff
October 18, 2021 4:19 PM, "Dmitri Smirnov" ***@***.*** ***@***.******@***.***>)> wrote:
@plexoos commented on this pull request.
------------------------------------
In StRoot/StMuDSTMaker/COMMON/StMuDstMaker.cxx (#160 (comment)):
@@ -114,6 +114,8 @@ #include "StMuMcTrack.h" #include "StG2TrackVertexMap.h" +#include <strstream> //added/needed to compile successfully after 01.Oct.21. Remove in merger if problem is solved in a different way. No further changes in file. Weidenkaff
Well, if you did merge into your branch then you should also push, otherwise your proposed change to include this deprecated header still shows in this PR
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#160 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AIXRR2W6FAEZBISOHNBP6S3UHQUHVANCNFSM5FJJ5EEQ).
Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).
|
added 6 commits
October 22, 2021 14:39
…ect header version after rebase without increment
|
Philipp, Have you resolved the slowness of the rebase? Do you need help from Dmitry or Hongwei on this? Thanks |
|
Hello everyone,
I pushed a version that should adress all the remaining comments from the reviewers. The slowness issue came from using git on an sshfs syncronised disk, so now i can at least avoid it in the future.
Best Regards
Philipp
October 25, 2021 7:01 PM, "Xin Dong" ***@***.*** ***@***.******@***.***>)> wrote:
Philipp,
Have you resolved the slowness of the rebase? Do you need help from Dmitry or Hongwei on this? Thanks
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#160 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AIXRR2TQEDJSCP236KZ4TLTUIWEN3ANCNFSM5FJJ5EEQ).
Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).
|
veprbl
reviewed
Oct 29, 2021
…base. Decremented etof collection ClassDef for real this time
|
Do we need another review/approval from one of the code onwers so the merge can be initiated? |
nigmatkulov
reviewed
Nov 2, 2021
|
|
||
| //_________________ | ||
| bool StPicoEvent::eTofGoodEventFlag( UShort_t iSector, UShort_t iModule, UShort_t iCounter ) const { | ||
| if( iSector < 13 || iSector > 24 ){ |
There was a problem hiding this comment.
Hi @PhilippWeidenkaff , I would suggest to change some hardcoded numbers later.
There was a problem hiding this comment.
Overall, looks okay and could be merged.
|
Grigory, if you can approve the PR so we can merge this in. Thanks |
jdbrice
approved these changes
Nov 4, 2021
plexoos
changed the title
|
It appears that several nightly test jobs failed last night after this commit. Issue #181 opened. |
jml985
pushed a commit
that referenced
this pull request
Dec 7, 2021
* fixed 64b integer conversion in getField() method and added additional setters from etofIn001 version of this file. Needed to unpack Get4Status bit into event files * merged with group development to have support for local run-by-run calibrations in the repository version. Nessecary only for creating calibrations, but should be available in the offical code. Added calculation of goodEventFlag for every counter from Get4 missmatch bits and pulser digis. * merged with group development to have support for local run-by-run calibrations in the repository version. Nessecary only for creating calibrations, but should be available in the offical code. Added calculation of goodEventFlag for every counter from Get4 missmatch bits and pulser digis. * <strstring.h> header include needed to be added for local compilation (?) * added eTofGoodEventFlag to mark counters which are good foer analysis in each event * moved struct to public to avoid root6 issue. Changed class dev version back to 0 * added kStFatal return in ::init if calib histo files are corrupt * Changed class dev version back to 0 * Trying to fix merger * Trying to fix merger * added eTofGoodEventFlag to mark counters which are good foer analysis in each event * moved struct to public to avoid root6 issue. Changed class dev version back to 0 * added kStFatal return in ::init if calib histo files are corrupt * Changed class dev version back to 0 * decremented classDef version of StEtofCollection. Makers compile correct header version after rebase without increment * removed double line in MuEtofHeader constructor * included eTOF constants header and moved constants here * included eTOF constants header and moved constants here * added constant nbGet4s here for use in event headers * Removed strstr include in StMuDstMaker.cxx. Now indentical to repository version * Moved vector init in eTOF headers to initializer list * Changed default reference pulser setting in calib maker back to data base. Decremented etof collection ClassDef for real this time Co-authored-by: Philipp Weidenkaff <weidenkaff@rcas6005.rcf.bnl.gov> Co-authored-by: PhilippWeidenkaff <weidenkaff@physi.uni-heildeberg.de>
marrbnl
pushed a commit
that referenced
this pull request
Dec 8, 2021
* fixed 64b integer conversion in getField() method and added additional setters from etofIn001 version of this file. Needed to unpack Get4Status bit into event files * merged with group development to have support for local run-by-run calibrations in the repository version. Nessecary only for creating calibrations, but should be available in the offical code. Added calculation of goodEventFlag for every counter from Get4 missmatch bits and pulser digis. * merged with group development to have support for local run-by-run calibrations in the repository version. Nessecary only for creating calibrations, but should be available in the offical code. Added calculation of goodEventFlag for every counter from Get4 missmatch bits and pulser digis. * <strstring.h> header include needed to be added for local compilation (?) * added eTofGoodEventFlag to mark counters which are good foer analysis in each event * moved struct to public to avoid root6 issue. Changed class dev version back to 0 * added kStFatal return in ::init if calib histo files are corrupt * Changed class dev version back to 0 * Trying to fix merger * Trying to fix merger * added eTofGoodEventFlag to mark counters which are good foer analysis in each event * moved struct to public to avoid root6 issue. Changed class dev version back to 0 * added kStFatal return in ::init if calib histo files are corrupt * Changed class dev version back to 0 * decremented classDef version of StEtofCollection. Makers compile correct header version after rebase without increment * removed double line in MuEtofHeader constructor * included eTOF constants header and moved constants here * included eTOF constants header and moved constants here * added constant nbGet4s here for use in event headers * Removed strstr include in StMuDstMaker.cxx. Now indentical to repository version * Moved vector init in eTOF headers to initializer list * Changed default reference pulser setting in calib maker back to data base. Decremented etof collection ClassDef for real this time Co-authored-by: Philipp Weidenkaff <weidenkaff@rcas6005.rcf.bnl.gov> Co-authored-by: PhilippWeidenkaff <weidenkaff@physi.uni-heildeberg.de>
plexoos
added a commit
that referenced
this pull request
Oct 24, 2022
Fixes bug introduced in #160 Co-authored-by: PhilippWeidenkaff <weidenkaff@physi.uni-heildeberg.de> Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds a flag for each counter to the eTOF headers and StPicoEvent. The flag shows if a counter is good for analysis in a given event. Should be implemented before production of 2020 data.
Additional changes to the StETofCalibMaker have been made to include features needed for local calibrations into the official code and introduce additional monitoring (neither of this is relevant to production running).
in StMuDstMaker, <strstring.h> header was included to be able to compile it. This is not an intentional change on my side and if this issue has been fixed in another way, please ignore it.