Fst QA and Calibration Makers

[jdbrice]

This pull request builds on PR#266 and is needed for offline QA.

[starsdong]

Hi Daniel,

Thanks for submitting this request. To clarify, the request includes the following new makers

StFstCalibrationMaker
StFstClusterMaker
StFstHitMaker
StFstQAMaker

• corresponding changes in StFstDb and StFstUtil

How much of these are included in PR. 266 or all these are exclusively the additional update to PR. 266?

It seems there is some compiling error in the CI test. Would be good to take a look and fix it.

In addition to the existing code owners to review these, I will ask a couple of colleagues to review the new makers.

Thanks and Regards

/xin

[starsdong]

Sorry, ClusterMaker and HitMaker are in PR. 266. So this request is for

StFstCalibrationMaker
StFstQAMaker

Will follow along with reviewers. Thanks

[genevb]

Unless there is a novel use case here, we typically don't save instances of maker classes into files. So the version for ClassDef should be 0 to avoid unnecessary functions and schema evolution relevant for I/O of this class.

[genevb]

Just looking at StFstQAMaker class:

• ClassDef version
• The header file includes several unnecessary header files. For example, one only needs class TH2F; because that's all that's needed for TH2F pointers, and #include "TH2F.h" can be in the implementation (.cxx) file. Not even sure why TNtuple.h was included, when class TTree; is all that is needed here.
• The Finish() function writes histograms to a file by default. If this is a maker we will run in official productions, (1) we don't want anyone writing additional files that will just be discarded anyhow, and (2) histograms should go to the .hist.root files that production already generates. The latter should happen automatically for histograms instantiated in Init(). If you want the histograms (or anything) written to a different file, that should be an option that can be switched on, but not as default. I don't think TTrees can go to the hist.root file, so if you want to output TTrees during official productions, we'll need to re-think things considerably.
• There's a mixture of gMessMgr and LOG_INFO usage. The recommendation is to just use LOG_INFO, LOG_WARN, etc.. And check that you terminated these messages with endm, not endl.

Thanks,
-Gene

[starsdong]

Daniel, now the PR#266 is merged. This PR contains also files from those in the PR#266 request. I think you would need to strip out others and keep on StFstCalibrationMaker and StFstQAMaker (+some needed other changes) in this request.

[starsdong]

Hi Daniel and FST experts, will you be able to address Gene's comments and also resolve the CI Build failure (missing StFstUtil/StFstConstant.h" file)?

[sunxuhit]

Hi Xin, I was able to fix the issue and compiled the code under my rcf account. But somehow the ROOT6 test on GitHub failed. Any suggestions?

[sunxuhit]

Just looking at StFstQAMaker class:

• ClassDef version
• The header file includes several unnecessary header files. For example, one only needs class TH2F; because that's all that's needed for TH2F pointers, and #include "TH2F.h" can be in the implementation (.cxx) file. Not even sure why TNtuple.h was included, when class TTree; is all that is needed here.
• The Finish() function writes histograms to a file by default. If this is a maker we will run in official productions, (1) we don't want anyone writing additional files that will just be discarded anyhow, and (2) histograms should go to the .hist.root files that production already generates. The latter should happen automatically for histograms instantiated in Init(). If you want the histograms (or anything) written to a different file, that should be an option that can be switched on, but not as default. I don't think TTrees can go to the hist.root file, so if you want to output TTrees during official productions, we'll need to re-think things considerably.
• There's a mixture of gMessMgr and LOG_INFO usage. The recommendation is to just use LOG_INFO, LOG_WARN, etc.. And check that you terminated these messages with endm, not endl.

Thanks, -Gene

Hi Gene,

I believe your comments are solved in the most recent commit ae03ed8
Could you please have another look at it?

Best,

Xu

[genevb]

Hi Gene,

I believe your comments are solved in the most recent commit ae03ed8 Could you please have another look at it?

Hi,

Some comments have been addressed, yes (and thanks), but not completely. For example, why include these in the header file?

#include "StIOMaker/StIOMaker.h"
#include "StEvent/StEnumerations.h"
#include "StEvent/StFstConsts.h"

And then in StFstCalibrationMaker, why include these in the header file?

#include "TH2S.h"
#include "StEvent/StFstConsts.h"

Otherwise, I don't see anything obvious to point out.

-Gene

[sunxuhit]

Hi Gene,
I believe your comments are solved in the most recent commit ae03ed8 Could you please have another look at it?

Hi,

Some comments have been addressed, yes (and thanks), but not completely. For example, why include these in the header file?

#include "StIOMaker/StIOMaker.h"
#include "StEvent/StEnumerations.h"
#include "StEvent/StFstConsts.h"

And then in StFstCalibrationMaker, why include these in the header file?

#include "TH2S.h"
#include "StEvent/StFstConsts.h"

Otherwise, I don't see anything obvious to point out.

-Gene

Hi Gene,

#include "StEvent/StFstConsts.h" is necessary for code to compile. Everything else is deleted from the most recent commit.

Best,

Xu

[genevb]

#include "StEvent/StFstConsts.h" is necessary for code to compile. Everything else is deleted from the most recent commit.

I'll not let this hold up the PR any further, so I'll accept that. I just don't see any of the variables/constants defined in StFstConsts.h used in StFstCalibrationMaker.h. Thanks for making an effort to minimize the dependencies.

-Gene

[starsdong]

Are these any further comments to this PR? If no, I would suggest we close the review and merge this PR to main.

[starsdong]

I would call the conclusion of this review. Dmitri, could you please help merge this PR into main?