GMT #680

nigmatkulov · 2024-04-10T14:52:18Z

Commit GMT-related classes and macroses. Also update StarDb/geometry for GMT with the last alignemnt.

plexoos · 2024-04-10T16:39:19Z

StRoot/StGmtAligner/EventT.cxx

+  TH2F * dMin   = new TH2F("DMin","vumin",100,-0.75,0.75,100,-0.75,0.75); 
+  TH1F * vMin   = new TH1F("VMin","vmin", plotDuv.nx, plotDuv.xmin, plotDuv.xmax);
+  TH1F * uMin   = new TH1F("UMin","umin", plotDuv.nx, plotDuv.xmin, plotDuv.xmax);
+  TH1F * uMinC  = new TH1F("UMinC","umC", plotDuv.nx, plotDuv.xmin, plotDuv.xmax);


How is this histogram used? The compiler complains about the variable not being used

plexoos · 2024-04-10T16:51:29Z

StRoot/StGmtAligner/EventT.h

+     if (tree == 0) {
+       TFile *f = (TFile*)gROOT->GetListOfFiles()->FindObject("/star/data09/calib/fisyak/Pass112/TpcSsd/065/Event_6065045_raw_1010001.root");
+       if (!f) {
+         f = new TFile("/star/data09/calib/fisyak/Pass112/TpcSsd/065/Event_6065045_raw_1010001.root");


It is not a good practice to have hard coded file names in the code. A better option would be to introduce a function argument with a default value.

klendathu2k

Few suggestions indicated. Few changes should be made,... StGmtAligner should be a maker, and prefer c++ native types (e.g. int) over ROOT types (Int_t) when a class is not intended for IO.

klendathu2k · 2024-04-11T16:03:00Z

StRoot/StAnalysisMaker/StAnalysisMaker.cxx

@@ -1141,13 +1141,14 @@ void StAnalysisMaker::summarizeEvent(StEvent *event, Int_t mEventCounter) {
  if (event->numberOfPsds()) {
    LOG_QA << "# PSDs:                " << event->numberOfPsds() << endm;
  }
-#ifdef _ST_GMT_HIT_H_
+//#ifdef _ST_GMT_HIT_H_


I suggest removing rather than commenting ifdefs.

Removed most of those and left only a few that are needed for debug and analysis purpose

klendathu2k · 2024-04-11T16:33:18Z

StRoot/StEvent/StGmtHit.h

+  /// ADC error in Y
+  Float_t getErrorAdcY() const { return mdAdcY; }
+  /// Local X coordinate
+  Float_t getLocalX() const    { return position().x(); }


The optimizer probably avoids a copy here... but for consistency with the way you write the setters, I recommend using mPosition.x() rather than position().x(). (same for error and y...)

Agreed, but that cannot be done here due to the inheritance

klendathu2k · 2024-04-11T16:38:21Z

StRoot/StEvent/StGmtPoint.cxx

+     return;
+   }
+
+   mHitLocalX = mHitLocalY = 0;


Shouldn't lines 35 to 42 be uncommented? If not, what does this constructor do?

klendathu2k · 2024-04-11T16:42:08Z

StRoot/StEvent/StGmtStrip.h

+  Short_t mMaxAdcTB;
+  /// Max over the time bins
+  Short_t mMaxPedSubtractedAdcTB;
+  // Short_t mClusterSeedType;      // See types in StGmtConsts.h


Remove commented out references to mClusterSeedType (or uncomment them if they are needed.)

klendathu2k · 2024-04-11T16:43:11Z

StRoot/StEvent/StGmtStripCollection.h

+    // strip.  New strip is created if it does not exist, but only
+    // using StGmtStrip() constructor.  Ownership is retained by the
+    // collection.
+//     StGmtStrip* getStrip( int elecId );


klendathu2k · 2024-04-11T16:43:54Z

StRoot/StEvent/StGmtStripCollection.h

+
+// inline functions
+
+// inline StGmtStripCollection::StGmtStripCollection( short module ) : StObject(), mModule( module ) {


... cleanup constructors / methods / variables which are commented out.

klendathu2k · 2024-04-11T16:47:05Z

StRoot/StEvent/StGmtStripCollection.h

+};
+
+// sort by coordinate number
+inline void StGmtStripCollection::sortByCoord(){


Just a comment. You can always sort vectors inside of consumer codes. So... I don't really see a need to sort the in-memory collection, unless there are multiple downstream codes that would prefer a given order.

klendathu2k · 2024-04-11T16:53:44Z

StRoot/StGmtAligner/StGmtAligner.cxx

+#include "StTpcDb/StTpcDb.h"
+#include "StDetectorDbMaker/StGmtSurveyC.h"
+//________________________________________________________________________________
+StGmtAligner::StGmtAligner(const Char_t *name) : StMaker(name),fFile(0), fTree(0), fEvent(0) {


StGmtAligner is a maker. The "maker" rules should apply here.

The name of the code should by in the form StTLAMaker. e.g. StGmtAlignmentMaker

The code should be organized under StRoot/StGmtAlignmentMaker/

klendathu2k · 2024-04-11T16:54:48Z

StRoot/StGmtAligner/runGmtTree.C

@@ -0,0 +1,23 @@
+void runGmtTree(const Char_t *input, const Char_t *output=0) {


add a "macros" directory to collect analysis macros.

klendathu2k · 2024-04-11T16:56:46Z

StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx

+#include "StMessMgr.h" 
+ClassImp(StGmtClusterMaker);
+
+Int_t StGmtClusterMaker::gmtStat = 0;


STAR coding standards prefer using standard c++ types (int, float, char, etc...) over ROOT types (Int_t, Float_t, etc...) when you are implementing a non persistent class... i.e. anything you don't intend save in MuDst, PicoDst, StEvent... or any ttree.

plexoos

Before I do another round of comments on this massive PR, I would like to see the compiler errors addressed.

plexoos · 2024-05-29T15:28:25Z

StRoot/StEvent/StGmtHit.h

+  Float_t mChargeUncert;
+
+private:   
+  ClassDef(StGmtHit,2)


StGmtHit is a new data type. Why start with version 2?

plexoos · 2024-05-29T15:29:18Z

StRoot/StEvent/StGmtHitCollection.cxx

+#include "StGmtHit.h"
+#include "StGmtHitCollection.h"
+
+ClassImp(StGmtHitCollection)


The ClassImp macro is not needed anywhere. Please remove

plexoos · 2024-05-29T15:34:51Z

StRoot/StEvent/StGmtPoint.cxx

+   if ( !hit1 || !hit2 ) {
+     LOG_ERROR << "Passed null pointer into StGmtPoint::StGmtPoint( StGmtHit* hit1, StGmtHit* hit2, int key )" << endm;
+     mKey = -999;
+     return;


You can't return from a constructor. It must create a valid object or throw an exception. Please reconsider the logic. You can make sure the input StGmtHits are valid to construct a StGmtPoint

plexoos · 2024-05-29T15:35:43Z

StRoot/StEvent/StGmtStrip.cxx

+// StGmtStrip header
+#include "StGmtStrip.h"
+
+ClassImp(StGmtStrip)


remove

Suggested change

ClassImp(StGmtStrip)

plexoos · 2024-05-29T15:40:00Z

StRoot/StEvent/StGmtStripCollection.cxx

+//     if( (h1->isY() && h2->isY()) || ( !(h1->isY()) && !(h2->isY()) ) )
+//     {
+// //       LOG_ERROR << "StGmtStripCollection::hitCoordLessThan sort CASE 1: " << 
+// //       "\n\t h1 " << h1 << 
+// //       "\n\t h1->isY() " << h1->isY() << 
+// //       "\n\t h1->GetCoordNum() " << h1->getCoordNum() << 
+// //       "\n\t h2 " << h2 << 
+// //       "\n\t h2->isY() " << h2->isY() << 
+// //       "\n\t h2->GetCoordNum() " << h2->getCoordNum() << 
+// //       "\n\t will return: " << h1->getCoordNum() << " < " << h2->getCoordNum() << endm;
+//       return h1->getCoordNum() < h2->getCoordNum();
+//     }
+//     else if( h1->isY() && !(h2->isY()) )
+//     {
+// //       LOG_ERROR << "StGmtStripCollection::hitCoordLessThan sort CASE 2: " << 
+// //       "\n\t h1 " << h1 << 
+// //       "\n\t h1->isY() " << h1->isY() << 
+// //       "\n\t h1->GetCoordNum() " << h1->getCoordNum() << 
+// //       "\n\t h2 " << h2 << 
+// //       "\n\t h2->isY() " << h2->isY() << 
+// //       "\n\t h2->GetCoordNum() " << h2->getCoordNum() << 
+// //       "\n\t will return: " << h1->getCoordNum()+kGmtNumStrips << " < " << h2->getCoordNum() << endm;
+//       return (h1->getCoordNum()+kGmtNumStrips) < h2->getCoordNum(); // order X first 
+//     }
+//     else if( !(h1->isY()) && h2->isY() )
+//     {
+// //       LOG_ERROR << "StGmtStripCollection::hitCoordLessThan sort CASE 3: " << 
+// //       "\n\t h1 " << h1 << 
+// //       "\n\t h1->isY() " << h1->isY() << 
+// //       "\n\t h1->GetCoordNum() " << h1->getCoordNum() << 
+// //       "\n\t h2 " << h2 << 
+// //       "\n\t h2->isY() " << h2->isY() << 
+// //       "\n\t h2->GetCoordNum() " << h2->getCoordNum() << 
+// //       "\n\t will return: " << h1->getCoordNum() << " < " <<  h2->getCoordNum()+kGmtNumStrips << endm;
+//       return h1->getCoordNum() < (h2->getCoordNum()+kGmtNumStrips); // order X first 
+//     }
+//     else
+//     {
+// //       LOG_ERROR << "StGmtStripCollection::hitCoordLessThan sort FAILED: " << 
+// //       "\n\t h1 " << h1 << 
+// //       "\n\t h1->isY() " << h1->isY() << 
+// //       "\n\t h1->GetCoordNum() " << h1->getCoordNum() << 
+// //       "\n\t h2 " << h2 << 
+// //       "\n\t h2->isY() " << h2->isY() << 
+// //       "\n\t h2->GetCoordNum() " << h2->getCoordNum() << 
+// //       "\n\t will return: " << -1 << endm;
+//       return -1;
+//     }


Please remove this "dead" code

plexoos · 2024-05-29T15:45:32Z

StRoot/StGmtAligmentMaker/EventT.h

+     Init(tree);
+   }
+
+   virtual ~TBase() {if (!fChain) return; delete fChain->GetCurrentFile();}


How can you be sure that the current file is the one you opened in this maker? What if another maker makes a different file current?
Also, are you sure the file is written and closed when you call it destructor?

plexoos · 2024-05-29T15:48:04Z

StRoot/StGmtAligmentMaker/HitT.h

+#if 0
+  void SetValidDerivatives(Bool_t p=kTRUE) {fValidDerivatives = p;}
+  void SetDerivatives(Double32_t *der) {Double32_t *d = &duPdxV; for (Int_t i = 0; i < 12; i++) d[i] = der[i];}
+#endif


What's the point of disabling these lines with #if 0?

plexoos · 2024-05-29T15:54:30Z

StRoot/StGmtAligmentMaker/StGmtAligner.cxx

+#include <stdlib.h>
+#include "Riostream.h"
+#include "TROOT.h"
+#include "TSystem.h"
+#include "TFile.h"
+#include "TKey.h"
+#include "TRandom.h"
+#include "TTree.h"
+#include "TBranch.h"
+#include "TStopwatch.h"
+#include "StThreeVectorF.hh"
+#include "StMatrixF.hh"
+#include "TH1.h"
+#include "TH2.h"
+#include "TProfile.h"
+#include "TMath.h"
+#include "TVector3.h"
+#include "TProcessID.h"
+#include "StEvent.h"
+#include "StPrimaryVertex.h"
+#include "StBFChain.h"
+#include "TGeoMatrix.h"
+#include "EventT.h"
+#include "StTpcDb/StTpcDb.h"
+#include "StDetectorDbMaker/StGmtSurveyC.h"


Include only the header files you really need.

fgeurts · 2024-08-21T15:59:19Z

Hi, what is the status of this PR? Have all comments been addressed? If so, can we wrap this one up and prepare this to be merged into the STAR library?

StarDb added

591afa9

nigmatkulov requested review from genevb, klendathu2k, plexoos, fisyak, R-Witt and iraklic as code owners April 10, 2024 14:52

plexoos reviewed Apr 10, 2024

View reviewed changes

klendathu2k requested changes Apr 11, 2024

View reviewed changes

plexoos requested changes Apr 11, 2024

View reviewed changes

Grigory Nigmatkulov and others added 10 commits May 22, 2024 12:34

Addressing stylish comments

7b302dd

Revert changes due to the inheritance issue

0a15563

Fixing EventT

875a7e4

Moving from the ROOT to C++ type declaration

20dffdf

Update missing parts

b1fb23b

Commenting out unused histo

6ad10c0

Stylish modifications

be2bf1c

Fix typo

b8ebc77

Make preprocessor directive to hide code parts

48a7271

Trying to clean up more

6728fed

plexoos reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GMT #680

GMT #680

nigmatkulov commented Apr 10, 2024

plexoos Apr 10, 2024

plexoos Apr 10, 2024

nigmatkulov May 29, 2024

klendathu2k left a comment

klendathu2k Apr 11, 2024

nigmatkulov May 29, 2024

klendathu2k Apr 11, 2024

nigmatkulov May 29, 2024

klendathu2k Apr 11, 2024

klendathu2k Apr 11, 2024

nigmatkulov May 29, 2024

klendathu2k Apr 11, 2024

klendathu2k Apr 11, 2024

klendathu2k Apr 11, 2024

klendathu2k Apr 11, 2024

nigmatkulov May 29, 2024

klendathu2k Apr 11, 2024

klendathu2k Apr 11, 2024

nigmatkulov May 29, 2024

plexoos left a comment

plexoos May 29, 2024

plexoos May 29, 2024

plexoos May 29, 2024

plexoos May 29, 2024

plexoos May 29, 2024

plexoos May 29, 2024

plexoos May 29, 2024

plexoos May 29, 2024

fgeurts commented Aug 21, 2024


		// inline functions

		// inline StGmtStripCollection::StGmtStripCollection( short module ) : StObject(), mModule( module ) {

		@@ -0,0 +1,23 @@
		void runGmtTree(const Char_t input, const Char_t output=0) {

GMT #680

Are you sure you want to change the base?

GMT #680

Conversation

nigmatkulov commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klendathu2k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plexoos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgeurts commented Aug 21, 2024