Closed benkrikler closed 10 years ago
Hi Ben.
I've only had a look at the changes you made to TAPAlgorithms (I assume the rest is correct). It seems that we now store the amplitude and time in the struct and, for MaxBin, we store the time when we calculate the amplitude as well. What happens if we don't want MaxBinTime but use something else, will it be overwritten? Could we end up with bugs where people call CFTime first, then MaxBinAmplitude and end up using the wrong time?
For clarity's sake, I would prefer to keep MaxBinTime and MaxBinAmplitude separate. I don't think it's a major source of slowdown in our code at the moment but I haven't used rootana in a while so I may be wrong.
Hey Andy,
(I assume the rest is correct)
Sounds risky :p
What happens if we don't want MaxBinTime but use something else, will it be overwritten? Could we end up with bugs where people call CFTime first, then MaxBinAmplitude and end up using the wrong time?
No, to the first question and no to the second provided the person has some sense of what they're doing. I can't think how you would over-write a previously set value of the time since you would have to consciously ask for the time from the MaxBin method. The operator::()
is unchanged in that it still just returns the amplitude. If I then want to get the corresponding time for this peak I could then do something like:
double time = fMaxBin->time;
but there's no need to do this and certainly there's no risk of 'over-writing' a value previously set by some other method.
On the other hand, it seems pointless that you would run the same algorithm twice to get information already available after the first run, even if it does run quickly.
If you're concerned that the access to the time from the algorithm is through a public data member rather than a public getter then I'd be happy to change that.
provided the person has some sense of what they're doing
Sounds risky :p
Joking aside, I didn't fully understand the changes before but now it seems perfectly fine to me (although I would still prefer all algorithms to be used in the same way but let's just leave it as it is and I can adapt).
This has all been merged in now.
This is to merge the work with template fitting and convolution back into develop. On the whole I think this work was isolated from other people's work however I have changed a few parts of common code so I wanted to check everyone was aware and ok with these changes.
The changes to core (framework) bits of code:
EventNavigator::GetSetupRecord
is now a static method so you don't need to get the EventNavigator Instance first.