fix #270 - memory allocation for the autocorrelation in transient*

[tremblap]

allocates the maximum ARMmodel size so it fulfills the conditions of resizing (line 446)

[AlexHarker]

"so it fulfills the conditions of resizing" is very vague. The two changes I see are:

1 - to pass the result of resizing of the ARModel as an unsigned number - ARModel takes an index, so I'm not certain that this is strictly necessary, although arguably ARModel should handle sizes internally with some correct behaviour for untoward input (if the input is positive this will have no effect).

2 - To add the model order to the resize. I have no idea why this is seen as necessary - I don't see an obvious rationale for it, but that doesn't mean it's wrong - there's quite a bit here and in ARModel that is likely under documented so seeing what is correct is difficult. However, it's clear that analysisSize() is what should get passed to the ARModel and that would point to the size that was previously set.

Can you outline the reason for the changes, and whether this fixes a specific issue - there is no link to an issue that I can see so it's unclear why these specific changes have been made.

[tremblap]

as the title says, fixes 270. Assert crash when the conditions, again said above (line 446) are not met. For instance, when order is larger than pad and that goes above the next power of two.

If the algo is right at line 446, then the requested size was wrong, so I corrected the latter.

[tremblap]

sadly this doesn't solve (as I hoped) #289 so there is now an issue there.

[AlexHarker]

You will note that GitHub doesn't link issues mentioned in the title - it would be clearer to put things in the body text of the merge request. I would also advise against "this doesn't fix this issue" comments unless they undo an earlier comment on this merge request, as this just links out to an unrelated issue now.

If the algo is right at line 446, then the requested size was wrong, so I corrected the latter.

I assume you mean in the file edited? That doesn't appear necessarily correct to me as a statement. See this line which sends only analysis size through to the model.

flucoma-core/include/algorithms/public/TransientExtraction.hpp

Line 156 in 8539bcd

mModel.estimate(FluidTensorView<const double, 1>(

(note that clang-format has split the line of code so you will need to go through to the link to see the relevant info)

If you are not verbose in your comments it is much harder to fulfil a review meaningfully. There's obviously an issue if an assert is firing, but at the moment I'm not sure where the issue is as there are at least 3 files involved, and if I understand correctly the assert might be in the FFT file.

[tremblap]

I was hoping that this fix (which does fix #270 as long as we don't go up with any of the parameters after initialisation) would also fix #289 - it doesn't. Sorry if I was unclear. The current fix (#288) is valid for #270 but we need a way to cap the 3 interlocking values at runtime. I have not included that latter solution yet as I don't know if/how our parameter checks can do this.

[AlexHarker]

Suggest to close this request as covered in #270 as not the correct fix for the issue.

[weefuzzy]

Superseded by #290