ara-software / AraRoot

ARA data access framework
2 stars 7 forks source link

Addition of Getter function for arrival times in the RayTraceCorrelator #61

Closed jflaherty13 closed 6 months ago

jflaherty13 commented 6 months ago

Created getter function for arrival times called LookupArrivalTimes(), which simply allows access to the private variable arrivalTimes_. For consistency, I also updated a step in the correlator where it looks up the arrival times using the private variable to now use the getter function.

clark2668 commented 6 months ago

Thanks! Can I ask: why did you choose to handle this with a pass-by-reference rather than returning the double directly?

That is, why:

void RayTraceCorrelator::LookupArrivalTimes(
    int ant, int solNum,
    int thetaBin, int phiBin,
    double &arrivalTime
){

and not

double RayTraceCorrelator::LookupArrivalTimes(
    int ant, int solNum,
    int thetaBin, int phiBin){
   // do stuff
   return arrivalTime;
}

For readability reasons the latter is preferred.

jflaherty13 commented 6 months ago

Thanks! Can I ask: why did you choose to handle this with a pass-by-reference rather than returning the double directly?

That is, why:

void RayTraceCorrelator::LookupArrivalTimes(
    int ant, int solNum,
    int thetaBin, int phiBin,
    double &arrivalTime
){

and not

double RayTraceCorrelator::LookupArrivalTimes(
    int ant, int solNum,
    int thetaBin, int phiBin){
   // do stuff
   return arrivalTime;
}

For readability reasons the latter is preferred.

I mainly did it to keep it in line with the syntax of LookupArrivalAngles function:

void RayTraceCorrelator::LookupArrivalAngles( int ant, int solNum, int thetaBin, int phiBin, double &arrivalTheta, double &arrivalPhi ){ arrivalTheta = this->arrivalThetas[solNum][thetaBin][phiBin][ant]; arrivalPhi = this->arrivalPhis[solNum][thetaBin][phiBin][ant]; }

clark2668 commented 6 months ago

Ahh, I see. I would switch it to the return version, if you don't mind. I chose pass by reference for LookupArrivalAngles specifically because I wanted to get two variables out (theta and phi), and C++ doesn't support returning >1 variable in a return statement. But otherwise I generally don't like it.

clark2668 commented 6 months ago

Also, please add doxygen documentation to LookupArrivalTimes in the header file. You can used LookupArrivalAngles as a template. (I know it seems pedantic, but if we don't write them now we'll never have them!)

clark2668 commented 6 months ago

This looks good to me, thanks!