Review StiIst

StIstClusterMaker.h
===================

 ClassDef(StIstClusterMaker, 1) ==> ClassDef(StIstClusterMaker, 0)
  It is not too important, but we always use 0 for makers. We are not
  going to write makers
  
StIstClusterMaker.cxx
=====================

StIstClusterMaker::mTimeBin initialised as 0xFF and never changed. It could be more clear to define it as a constant, like enum {kSomething = 0xFF;} 
the same mSplitCluster which is =1
  
   UShort_t numRawHits = rawHitCollectionPtr->getNumRawHits();
   LOG_DEBUG << "Number of raw hits found in ladder " << (short) (ladderIdx + 1) << ": " << numRawHits << endm;

Why numRawHits is UShort_t? It is more clear and simple to use int.
Then you do not need any casts.

StIstScanClusterAlgo.cxx
========================

unsigned char clusterType = kIstScanClusterAlgo; unsigned char maxTb = -1, usedTb = -1; unsigned char ladder = 0, sensor = 0;
Why unsigned char? It is simpler and faster to use int.

  //get number of time bin used in this event
   static unsigned char nTimeBins = istCollection.getNumTimeBins();

1. Why again unsigned char, not int. Int is even faster and more clear
2. Why static? That means that nTimeBins will be initialised only once
   per run, not per event. But in comments it told that thi is per event
  
   So it is very like a BUG.
  
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------  
Int_t StIstScanClusterAlgo::doClustering(const StIstCollection &istCollection, StIstRawHitCollection &rawHitsOriginal, StIstClusterCollection &clusters )
...
//sort raw hits in increasing order by geometry ID
   rawHitsOriginal.sortByGeoId();
  
Why sorting? It is not used afterwards at all. Waisting of time.

----------------------------------------------------------------------------------------------------------------------------------------------------
std::vector<StIstRawHit *> rawHitsVec[kIstNumSensorsPerLadder][kIstNumColumnsPerSensor];
std::vector<StIstCluster *> clustersVec[kIstNumSensorsPerLadder][kIstNumColumnsPerSensor];
and after:

rawHitsVec[sensorIdx][columnIdx].reserve(kIstNumRowsPerSensor);

All vectors have fixed size of kIstNumRowsPerSensor. So there is no any gain to use vectors. More simple and fast to use ordinary arrays, like:
 
StIstRawHit* rawHitsVec[kIstNumSensorsPerLadder][kIstNumColumnsPerSensor][kIstNumRowsPerSensor] = {{{0}}}; This array will be automatically filled by zeroes
---------------------------------------------------------------------------------------------------------------
if ( channelId < 0 || channelId >= kIstNumElecIds) rawHitsOriginal.getRawHitVec().erase( rawHitPtr ); --rawHitPtr;
Why you erase pointer fro array. it is extremely expencive operation.
You can easely call clear() after the loop, which is very fast.
Int_t StIstRawHitMaker::Make() ============================== static Int_t ntimebin = mCurrentTimeBinNum; Again static. Why? It will remember only first value StIstRawHit_hh ============== static UChar_t mDefaultTimeBin;
Is it really static? Is it common variable and the same for all hits ?
If it is so, then method

unsigned char StIstRawHit::getDefaultTimeBin() const { return mDefaultTimeBin;};
to rewrite:
static unsigned char StIstRawHit::getDefaultTimeBin() { return mDefaultTimeBin;}; or even better static int StIstRawHit::getDefaultTimeBin() { return mDefaultTimeBin;}; ----------------------------------------------------------------------------------------- float StIstRawHit::getCharge( unsigned char tb ) const
{
return mCharge[ (tb < 0 || tb >= kIstNumTimeBins) ? mDefaultTimeBin : tb ];
};

float StIstRawHit::getChargeErr( unsigned char tb ) const
{
return mChargeErr[ (tb < 0 || tb >= kIstNumTimeBins) ? mDefaultTimeBin : tb ];
};

Here all tb<0 never happens because unsigned. It is a BUG

And in total, ALL unsigned char in your software are redundant. The do not save space, they are slower
then int, and leads to problems like above.

StIstCluster.h
==============
In this class using of UChar_t is reasonable, to save memory, because clusters are many.
But if you really want to save memory, it is better to place your variable in differen order
instead of:

Int_t mKey; ///< Cluster unique label
UChar_t mLadderId; ///< Ladder id the cluster belongs to
UChar_t mSensorId; ///< Sensor id the cluster belongs to
Float_t mMeanRow; ///< Cluster's mean row
Float_t mMeanColumn; ///< Cluster's mean column
Float_t mTotCharge; ///< Charge sum of the cluster
Float_t mTotChargeErr; ///< rMS noise of the cluster
UChar_t mClusteringType; ///< Clustering algorithm type
UChar_t mMaxTimeBin; ///< Max ADC time bin index
UChar_t mNRawHits; ///< Cluster size
UChar_t mNRawHitsRPhi; ///< Cluster size in r-phi direction
UChar_t mNRawHitsZ; ///< Cluster size in beam direction
UShort_t mIdTruth; //!< For embedding, 0 as background

it is better , for 3 bytes less:

Int_t mKey; ///< Cluster unique label
Float_t mMeanRow; ///< Cluster's mean row
Float_t mMeanColumn; ///< Cluster's mean column
Float_t mTotCharge; ///< Charge sum of the cluster
Float_t mTotChargeErr; ///< rMS noise of the cluster
UShort_t mIdTruth; //!< For embedding, 0 as background
UChar_t mLadderId; ///< Ladder id the cluster belongs to
UChar_t mSensorId; ///< Sensor id the cluster belongs to
UChar_t mClusteringType; ///< Clustering algorithm type
UChar_t mMaxTimeBin; ///< Max ADC time bin index
UChar_t mNRawHits; ///< Cluster size
UChar_t mNRawHitsRPhi; ///< Cluster size in r-phi direction
UChar_t mNRawHitsZ; ///< Cluster size in beam direction