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?

plexoos · 2024-11-28T02:09:02Z

The error you are getting is valid:

#10 1262.0 g++ -m64 -fPIC -pipe -Wall -Woverloaded-virtual -std=c++0x -Wno-long-long -pthread -Wno-deprecated-declarations -Werror -O2 -g -falign-loops -falign-jumps -falign-functions -Dsl79_gcc485 -D__ROOT__ -DNEW_DAQ_READER -I. -IStRoot -I.sl79_gcc485/include -I/opt/software/linux-scientific7-x86_64/gcc-4.8.5/root-5.34.38-ffhsnriu5ktoo4tieuku5tbezanz6q5g/include -c .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx -o .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.o
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx: In static member function 'static Int_t StGmtGeom::translateGeoNameToGeoId(const string&)':
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'rdo' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#10 1262.5      return encodeGeoId( rdo, arm, apv, channel );
#10 1262.5                                                 ^
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'arm' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'apv' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'channel' may be used uninitialized in this function [-Werror=maybe-uninitialized]

What do you expect to get from Int_t StGmtGeom::encodeGeoId(Int_t rdo, Int_t arm, Int_t apv, Int_t channel) if you don't provide values for its arguments?

star-sw/StRoot/StGmtUtil/geometry/StGmtGeom.cxx

Lines 149 to 160 in 79cd54e

    
           Int_t StGmtGeom::translateGeoNameToGeoId(const std::string & geoName) { 
        
               Short_t module, strip; 
        
               Int_t layer; 
        
               Int_t rdo, arm, apv, channel; 
        
               //	Error message already taken care of in decodeGeoName. 
        
               if ( decodeGeoName( geoName, module, layer, strip ) < 0 ) 
        
           	return kGmtError; 
        
               // return encodeGeoId( module, layer, strip ); 
        
               return encodeGeoId( rdo, arm, apv, channel ); 
        
           }

nigmatkulov · 2024-11-28T04:27:13Z

The error you are getting is valid:
#10 1262.0 g++ -m64 -fPIC -pipe -Wall -Woverloaded-virtual -std=c++0x -Wno-long-long -pthread -Wno-deprecated-declarations -Werror -O2 -g -falign-loops -falign-jumps -falign-functions -Dsl79_gcc485 -D__ROOT__ -DNEW_DAQ_READER -I. -IStRoot -I.sl79_gcc485/include -I/opt/software/linux-scientific7-x86_64/gcc-4.8.5/root-5.34.38-ffhsnriu5ktoo4tieuku5tbezanz6q5g/include -c .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx -o .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.o
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx: In static member function 'static Int_t StGmtGeom::translateGeoNameToGeoId(const string&)':
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'rdo' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#10 1262.5      return encodeGeoId( rdo, arm, apv, channel );
#10 1262.5                                                 ^
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'arm' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'apv' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#10 1262.5 .sl79_gcc485/OBJ/StRoot/StGmtUtil/geometry/StGmtGeom.cxx:159:48: error: 'channel' may be used uninitialized in this function [-Werror=maybe-uninitialized]
What do you expect to get from Int_t StGmtGeom::encodeGeoId(Int_t rdo, Int_t arm, Int_t apv, Int_t channel) if you don't provide values for its arguments?

star-sw/StRoot/StGmtUtil/geometry/StGmtGeom.cxx

Lines 149 to 160 in 79cd54e

Int_t StGmtGeom::translateGeoNameToGeoId(const std::string & geoName) {

Short_t module, strip;

Int_t layer;

Int_t rdo, arm, apv, channel;

// Error message already taken care of in decodeGeoName.

if ( decodeGeoName( geoName, module, layer, strip ) < 0 )

return kGmtError;

// return encodeGeoId( module, layer, strip );

return encodeGeoId( rdo, arm, apv, channel );

}

Good catch. I'll update this part

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

nigmatkulov added 7 commits November 27, 2024 09:45

Some updates regardsing the requests

65d159b

Update of char -> const char conversion

8604b50

Reshuffling function definition and remove unused variable

0c37f47

Remoing unused variables from StGmtRawMaker

f4fe3b0

Remove unused variables

03408dd

Merge branch 'main' into GMT

7355aec

More cleanup of StGmtGeom.cxx

ca072bb

nigmatkulov added 4 commits November 27, 2024 16:17

Check

f28e751

Fix of weird warning error

bd8f38f

Additional fix

4cb6ace

Add fix of the dummy error

79cd54e

nigmatkulov added 4 commits November 27, 2024 20:15

Set initial parameter values for static functions

856924a

Dummy fix fixed

57656c6

Add missed initial value for the argumetn

496e025

return function put into the braces

649d54e

Another minor fix

d23c8cb

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

plexoos commented Nov 28, 2024

nigmatkulov commented Nov 28, 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

plexoos commented Nov 28, 2024

nigmatkulov commented Nov 28, 2024