thaddeusdiamond/Social-Hierarchical-Learning — comment on commit 70a9f915d3f758f29a337227dace74f681fc6228
File: src/Observation/Changepoint/VarianceDetector.cc line 71
By hayesbh:

Hey Thad,

Here are responses to your e-mails:

1) Member vars in interfaces
- This doesn't seem like a big deal to me, but does lend itself to the possibility of someone redefining the vars in their implementation without realizing it. We can change it without taking much time, but for the sake of the new people I wanted to leave as little room to err as possible

2) I agree that all of the magic numbers should be turned to CONSTs at some point, but I was in a hurry and am not entirely sold on the way I implemented it as being the right way. If it shows some reasonable promise I'll go ahead and pretty it up, but right now I'd rather spend time on testing it.

3) I agree that passing around pointers is the quickest way to get it done, but again I see this as a fairly painless switch and don't see anticipate the size of these changepoint frame index vectors becoming problematic in the meantime.

4) No fabs(d) on max_first_derivative is a bug -- just committed the fix, but we don't want to push the abs onto the moving_variance_derivative -- good catch

5) I'm not worried about adding 0s to confidence, since that's only tracked as a debugging feature for me to see (it's not actually returned yet, and the offset is counted for the actual frame index of the changepoint).

6) Basically, recording the changepoint at the start of the window just indicates that a changepoint occurs within that window, not necessarily precisely where. I don't think this will end up being a problem, but I suspect it'll surface either way once I run some tests on it.

Thanks for taking a look at this on a Friday night -- I really appreciate both it and the attention to detail you've kept throughout our work together.

--Brad

On Feb 18, 2012, at 12:22 AM, Thaddeus Diamond wrote:

> Should 0.75 be a class member threshold? Also, for the first offset values of confidence shouldn't we push back a zero? Otherwise confidences will not line up with the timestamp representing the actual changepoint, they'll line up with the beginning of the window, at the end of which a changepoint occurs... This actually seems like an okay design but it requires being a little more conscientious in accessing the confidences.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/thaddeusdiamond/Social-Hierarchical-Learning/commit/70a9f915d3f758f29a337227dace74f681fc6228#commitcomment-980690