Closed jdbrice closed 2 years ago
Hi Daniel,
Thanks for submitting this request. To clarify, the request includes the following new makers
StFstCalibrationMaker StFstClusterMaker StFstHitMaker StFstQAMaker
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
Sorry, ClusterMaker and HitMaker are in PR. 266. So this request is for
StFstCalibrationMaker StFstQAMaker
Will follow along with reviewers. Thanks
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.
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)?
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?
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, whenclass 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
andLOG_INFO
usage. The recommendation is to just useLOG_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
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, 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,
Best,
Xu
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
Are these any further comments to this PR? If no, I would suggest we close the review and merge this PR to main.
I would call the conclusion of this review. Dmitri, could you please help merge this PR into main?
This pull request builds on PR#266 and is needed for offline QA.