Star-fst-l mailing list

[Daniel Brandenburg <dbrandenburg.ufl AT gmail.com>]

Hi All,

My apologies for not sending this sooner - I meant to send last week but forgot. Please find the *specific* comments to the FstFast simulator brought up during the integration review below. There are additional general comments, like consistent naming convention, use of stl, correct memory management etc. that also apply. All updates to the code since the start of the review are tracked here: https://github.com/jdbrice/star-fwd-integration (including all comments from Dmitri if you want to see them, and I will add raghav's specific comments once he submits them). 

-> I welcome any and all help addressing these issues via pull requests to that repo.

With that I want to take a moment to make something clear about the software that has come up a few times. As the software coordinator for the FWD upgrade I am the one ultimately responsible for this software and have the authority to change it as needed (again everything is public and version controlled for easy review). I fully support the goal to have others (not me) primarily maintain and be the experts on the Fst simulators but I still expect to contribute as part of a team - I consider myself part of the FST software group. I have made it clear that initially Zhenyu and Shenghui should have write access (others can be added later as needed)

I'll give another report once I have updated (or received updates to) these codes to comply with all of the integration requirements.

Best,

Daniel  

In StRoot/StFstSimMaker/StFstFastSimMaker.h:

> +class StRnDHitCollection;
+class StRnDHit;
+
+#include "StChain/StMaker.h"
+#include <vector>
+
+#include "TH1F.h"
+#include "TH2F.h"
+#include "TH3F.h"
+
+
+class StFstFastSimMaker : public StMaker {
+	public:
+		explicit StFstFastSimMaker(const Char_t *name = "fstFastSim");
+		virtual ~StFstFastSimMaker() {}
+		Int_t Make();

Use fundamental standard and ROOT types such as Int_t consistently. The general rule is to use the ROOT types for persistent data.

---

In StRoot/StFstSimMaker/StFstFastSimMaker.h:

> +	public:
+		explicit StFstFastSimMaker(const Char_t *name = "fstFastSim");
+		virtual ~StFstFastSimMaker() {}
+		Int_t Make();
+		int Init();
+		int Finish();
+		virtual const char *GetCVS() const;
+		
+		/// Set offset for each disk ( x=R*cos(idisk*60 degrees), y=R*sin(...) )
+		void setRaster(float R = 1.0) { mRaster = R; }
+
+		/// Set min/max active radii for each disk
+		void setDisk(const int i, const float rmn, const float rmx);
+		void setInEfficiency(float ineff = 0.1) { mInEff = ineff; }
+		void setQAFileName(TString filename = 0.1) { mQAFileName = filename; }
+		void setFillHist(const bool hist = false) { mHist = hist; }

If I remember correctly, class methods should start with a capital letter

---

In StRoot/StFstSimMaker/StFstFastSimMaker.cxx:

> @@ -0,0 +1,442 @@
+#include "StFstFastSimMaker.h"
+
+#include "St_base/StMessMgr.h"
+
+#include "StEvent/StEvent.h"
+#include "StEvent/StRnDHit.h"
+#include "StEvent/StRnDHitCollection.h"
+
+#include "tables/St_g2t_fts_hit_Table.h"
+#include "tables/St_g2t_track_Table.h"
+
+#include "StThreeVectorF.hh"

It is a good idea to prefix the header files with its package name so it is easier to identify where it is coming from.

---

In StRoot/StFstSimMaker/StFstFastSimMaker.cxx:

> +	StRnDHit *_map[NDISC][MAXR][MAXPHI];
+	double ***enrsum = (double ***)malloc(NDISC * sizeof(double **));
+	double ***enrmax = (double ***)malloc(NDISC * sizeof(double **));
+	
+	for (int id = 0; id < NDISC; id++) {
+		enrsum[id] = (double **)malloc(MAXR * sizeof(double *));
+		enrmax[id] = (double **)malloc(MAXR * sizeof(double *));
+		for (int ir = 0; ir < MAXR; ir++) {
+			enrsum[id][ir] = (double *)malloc(MAXPHI * sizeof(double));
+			enrmax[id][ir] = (double *)malloc(MAXPHI * sizeof(double));
+		}
+	}
+
+	for (int id = 0; id < NDISC; id++) {
+		for (int ir = 0; ir < MAXR; ir++) {
+			for (int ip = 0; ip < MAXPHI; ip++) {
+				_map[id][ir][ip] = 0;
+				enrsum[id][ir][ip] = 0;
+				enrmax[id][ir][ip] = 0;
+			}
+		}
+	}

What is the point of allocating all of these arrays dynamically? You do know all the dimensions beforehand. This hard-to-read code can be rewritten with a one dimensional std::array indexed by idx = i_max*(j_max*k + j) + i

---

In StRoot/StFstSimMaker/StFstFastSimMaker.cxx:

> +	const g2t_fts_hit_st *hit = hitTable->GetTable();
+	
+	StPtrVecRnDHit hits;
+
+	// track table
+	St_g2t_track *trkTable = static_cast<St_g2t_track *>(GetDataSet("g2t_track"));
+	if (!trkTable) {
+		LOG_INFO << "g2t_track table is empty" << endm;
+		return; // Nothing to do
+	}           // if
+
+	const Int_t nTrks = trkTable->GetNRows();
+	LOG_DEBUG << "g2t_track table has " << nTrks << " tracks" << endm;
+	const g2t_track_st *trk = trkTable->GetTable();
+
+	gRandom->SetSeed(0);

gRandom is a global variable which means you reset the seed for the entire framework. This should not be done by your library

---

In StRoot/StFstSimMaker/StFstFastSimMaker.cxx:

> +			float y = hit->x[1];
+			float z = hit->x[2];
+
+			if (z > 200)
+				continue; // skip large disks
+
+			// rastered
+			float xx = x - xc;
+			float yy = y - yc;
+
+			float r = sqrt(x * x + y * y);
+			float p = atan2(y, x);
+
+			// rastered
+			float rr = sqrt(xx * xx + yy * yy);
+			float pp = atan2(yy, xx);

What's the point of doing the majority of calculations in this routine in single precision?

---

In StRoot/StFstSimMaker/StFstFastSimMaker.cxx:

> +			if (MAXR)
+				assert(ir < MAXR);
+			if (MAXPHI)
+				assert(ip < MAXPHI);
+
+			StRnDHit *fsihit = 0;
+			if (_map[d - 1][ir][ip] == 0) // New hit
+			{
+
+				if (FstGlobal::verbose)
+					LOG_INFO << Form("NEW d=%1d xyz=%8.4f %8.4f %8.4f r=%8.4f phi=%8.4f iR=%2d iPhi=%4d dE=%8.4f[MeV] truth=%d",
+							d, x, y, z, r, p, ir, ip, e * 1000.0, t)
+						<< endm;
+
+					count++;
+				fsihit = new StRnDHit();

A possible memory leak