| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0035162 | Lazarus | TAChart | public | 2019-02-27 21:55 | 2019-03-02 23:54 |
| Reporter | Marcin Wiazowski | Assigned To | wp | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | 2.1 (SVN) | Product Build | 60526 | ||
| Target Version | 2.2 | Fixed in Version | |||
| Summary | 0035162: TAChart: improper initialization in TFitSeries.PrepareFitParams | ||||
| Description | TFitSeries can work in the following modes, that can chosen by using a FitEquation property: fePolynomial, // y = b0 + b1*x + b2*x^2 + ... bn*x^n feLinear, // y = a + b*x feExp, // y = a * exp(b * x) fePower, // y = a * x^b feCustom // y = b0 + b1*F1(x) + b2*F2(x) + ... bn*Fn(x), where F1(x) .. Fn(x) provided by the user Let's assume for a moment, that we have FitEquation = fePolynomial, and our interpolated function is: y = 1 + 2*x + 3*x^2 + 4*x^3 which may be written as: y = 1*x^0 + 2*x^1 + 3*x^2 + 4*x^3 which may be written as: y = 1*Power(x,0) + 2*Power(x,1) + 3*Power(x,2) + 4*Power(x,3) In TFitSeries internals, Power() is encapsulated as FitBaseFunc_Poly(): y = 1*FitBaseFunc_Poly(x,0) + 2*FitBaseFunc_Poly(x,1) + 3*FitBaseFunc_Poly(x,2) + 4*FitBaseFunc_Poly(x,3) To make a code more universal, a table has been introduced, containing functions to call - it is initialized in a TFitSeries.PrepareFitParams() method as: FFitParams[0].Func := @FitBaseFunc_Poly // calculates x^0 FFitParams[1].Func := @FitBaseFunc_Poly // calculates x^1 FFitParams[2].Func := @FitBaseFunc_Poly // calculates x^2 FFitParams[3].Func := @FitBaseFunc_Poly // calculates x^3 so we get: y = 1*FFitParams[0].Func(x,0) + 2*FFitParams[1].Func(x,1) + 3*FFitParams[2].Func(x,2) + 4*FFitParams[3].Func(x,3) Interestingly, exactly same calculation is used also for feLinear, feExp and fePower modes - only some additional calculations are applied earlier/later. And, finally, we have the feCustom mode. In this case, we must call a TFitSeries.SetFitBasisFunc() method multiple times, to set FFitParams[x].Func functions as we need. For example, in "tachart\demo\fit\fitdemo.lpr" demo, we have: FitSeries.SetFitBasisFunc(1, @HarmonicBaseFunc, 'sin(x)'); FitSeries.SetFitBasisFunc(2, @HarmonicBaseFunc, 'sin(3 x)'); FitSeries.SetFitBasisFunc(3, @HarmonicBaseFunc, 'sin(5 x)'); Now, let's reproduce the problem with the attached Reproduce application. There are two identical charts there, each having an identical TFitSeries series - with one exception: - left chart's series has initial setting FitEquation = fePolynomial, - right chart's series has initial setting FitEquation = feCustom. After launching the application, left chart shows a red curve, and right chart shows nothing (this is normal - SetFitBasisFunc() methods have not been called yet). Now press the "Test" button: it makes both the series identical - both receive same FitEquation = feCustom setting, and same SetFitBasisFunc() calls are made for both of them. The result is: left chart starts to show a new red curve's shape - but right chart still shows nothing. This problem can be also reproduced by using the "tachart\demo\fit\fitdemo.lpr" demo: - to avoid more advanced adjustments in the demo application, at the beginning of TfrmMain.FitCompleteHandler(), just place "exit", - to avoid more advanced adjustments in the demo application, in TfrmMain.FormCreate(), remove both "FitSeries.FitRange. ... := ..." lines, - launch the application, - select the "Fit equation" combo as "Harmonic" (so FitEquation = feCustom mode will be used), - now you can see a red series on the chart. Now modify the application, so it will be launched already with the "Harmonic" setting: - in TfrmMain.FormCreate(), change "cbFitEquation.ItemIndex := 0;" to "cbFitEquation.ItemIndex := 4;" (don't confuse with "cbTestFunction"), - in Object Inspector, change FitSeries.FitEquation from "fePolynomial" to "feCustom", - launch the application, - you will see NOTHING on the chart, - set the "Fit equation" combo to any other item, and then back to "Harmonic", - now you can see a red series on the chart - as it should be from the beginning. Explanation: In non-feCustom modes, all FFitParams[ 0 .. Max ].Func items are initialized internally, in the TFitSeries.PrepareFitParams() method. In feCustom mode, FFitParams[ 1 .. Max ].Func items are initialized by calling SetFitBasisFunc() from the user's code. But what about FFitParams[ 0 ].Func? If the series has been earlier set to some non-feCustom mode, FFitParams[ 0 ].Func is still initialized to @FitBaseFunc_Poly; but if not, FFitParams[ 0 ].Func is not initialized, so chart shows nothing. So maybe the user should also call SetFitBasisFunc(0, ...)? In this case, the compiler will warn us: "range check error while evaluating constants (0 must be between 1 and 2147483647)". This is because we should never change FFitParams[ 0 ].Func, and it should be always set to @FitBaseFunc_Poly. This is because FFitParams[0].Func must always return x^0 (which is in fact equal to 1) - this just leaves the first interpolation parameter (a constant value - it's 1 in our example: y = 1* ...) untouched - just as it is required by the interpolation algorithm. So the solution is: FFitParams[0].Func must be internally initialized to @FitBaseFunc_Poly even when FitEquation = feCustom. By the way, we can optimize the code a bit: currently, the initialization is: FFitParams[0].Func := @FitBaseFunc_Poly // calculates x^0 FFitParams[1].Func := @FitBaseFunc_Poly // calculates x^1 FFitParams[2].Func := @FitBaseFunc_Poly // calculates x^2 FFitParams[3].Func := @FitBaseFunc_Poly // calculates x^3 FFitParams[4].Func := @FitBaseFunc_Poly // calculates x^4 FFitParams[5].Func := @FitBaseFunc_Poly // calculates x^5 FFitParams[6].Func := @FitBaseFunc_Poly // calculates x^6 Instead of using FitBaseFunc_Poly to calculate even x^0 or x^1, we can use dedicated (and thus faster) functions: FFitParams[0].Func := @FitBaseFunc_Const // calculates x^0 FFitParams[1].Func := @FitBaseFunc_Linear // calculates x^1 FFitParams[2].Func := @FitBaseFunc_Square // calculates x^2 FFitParams[3].Func := @FitBaseFunc_Cube // calculates x^3 FFitParams[4].Func := @FitBaseFunc_Poly // calculates x^4 FFitParams[5].Func := @FitBaseFunc_Poly // calculates x^5 FFitParams[6].Func := @FitBaseFunc_Poly // calculates x^6 So, after this optimization, for the FitEquation = feCustom case, FFitParams[0].Func will be internally initialized to @FitBaseFunc_Const. The attached patch is quite simple; it solves the described problem and also: - removes one completely outdated comment, - updates another comment, - raises an exception if calling SetFitBasisFunc() when FitEquation <> feCustom - in this case, FFitParams[].Func items are initialized internally in TFitSeries.PrepareFitParams() and must not be changed. After applying the patch and pressing the "Test" button in the Reproduce application, also the right chart shows its series. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | 60549, 60554, 60555, 60558 | ||||
| LazTarget | 2.2 | ||||
| Widgetset | Win32/Win64 | ||||
| Attached Files |
| ||||
|
|
Reproduce.zip (2,674 bytes) |
|
|
Reproduce.png (6,667 bytes) |
|
|
patch.diff (2,217 bytes) |
|
|
Good catch. But who guarantees that the user wants to apply a custom fit which has the constant term at position 0 of the FitParams array? In fact, when you flip the array (polynomial ending with the constant term) the custom fit works too: Chart2FitSeries.SetFitBasisFunc(0, @FitBaseFunc_5, 'x^5'); Chart2FitSeries.SetFitBasisFunc(1, @FitBaseFunc_4, 'x^4'); Chart2FitSeries.SetFitBasisFunc(2, @FitBaseFunc_Cube, 'x^3'); Chart2FitSeries.SetFitBasisFunc(3, @FitBaseFunc_Square, 'x^2'); Chart2FitSeries.SetFitBasisFunc(4, @FitBaseFunc_Linear, 'x'); Chart2FitSeries.SetFitBasisFunc(5, @FitBaseFunc_Const, ''); with function FitBaseFunc_4(x: ArbFloat; Param: Integer): ArbFloat; begin Result := x*x*x*x; end; function FitBaseFunc_5(x: ArbFloat; Param: Integer): ArbFloat; begin Result := x*x*x*x*x; end; So, we cannot make any assumptions on fit function at index 0. What is wrong is the type TFitFuncIndex which should start with 0, not at 1, to avoid the compiler warning. This must be a left-over from the old version where custom fits were not possible. The other, more important issue is that the embedded TryFit routine does not set the FErrCode of the series, when PrepareFitParams fails. // Prepare fit parameters if not PrepareFitParams then begin FErrCode := fitNoBaseFunctions; // <-- was: fitRes.ErrCode := ... exit; end; With that fixed the user can check in the OnFitComplete event whether the fit was successful or not - in the case of the missing function pointer for the 0th basis function it would not be successful and the ErrCode would be "fitNoBaseFunctions". The problem left for me is that the OnFitComplete handler is called too often, I will have a look later. |
|
|
Reproduce2.zip (2,313 bytes) |
|
|
Related: I'm attaching a Reproduce2 test application. Tested with r60543: 1) Load Reproduce2 application in IDE - you will see a chart with red series 2) In Object Inspector, select Chart1FitSeries and change its FitEquation from fePolynomial to feCustom 3) Red series is still visible 4) Save the project, close the designed form and load the form again 5) Red series is no longer still visible The problem is that, after changing FitEquation to feCustom, red series should disappear immediately - because feCustom requires making calls to SetFitBasisFunc(), which is, of course, not performed when designing in IDE. The problem is in TFitSeries.InvalidateFitResults(): it resets FFitParams[x].Value, but not FFitParams[x].Func - so old functions are reused in the new (i.e. feCustom) mode; this is not good. Reloading the form finally makes the FFitParams table filled with zeros (i.e. completely empty), so series eventually disappears. When I added: for i:=0 to High(FFitParams) do FFitParams[i].Func := nil; to TFitSeries.InvalidateFitResults(), the problem disappeared. However, I'm not sure if resetting the functions should be made in all cases, when InvalidateFitResults() is called, or only sometimes. Please also see my post below. |
|
|
I can see two solutions. 1) I don't have anything against requiring the user to call also SetFitBasisFunc(0, ...). In this case, the change should be described as incompatible - with an explanation that, in most cases, the user will want to call: SetFitBasisFunc(0, @FitBaseFunc_Const, ''). 2) Maybe - instead in TFitSeries.PrepareFitParams() - FFitParams[x].Func should be initialized earlier, i.e. in the TFitSeries.InvalidateFitResults()? In this case, FFitParams[x].Func could be initialized for all modes, including feCustom. Thanks to that: - patch will NOT be incompatible, - not making any SetFitBasisFunc() calls will still work, by using the default functions - just as fePolynomial, - even in feCustom mode, some curve will be visible in IDE when designing. |
|
|
The problem is: When the user wants to fit a superposition of, say, sin(x) and sin(2 x) is he aware that there is a hidden basis function for the constant term at index 0? Does he wonder why he has to specify ParamCount as 3 although he provides only 2 functions? When he queries the fit results does he know how to get the constant value although he never specified it? Similar with the FixedParams needed when holding parameters constants - now again he explicitly must consider a parameter that he never specified. From this viewpoint I think that it is more logical when he has to add the basis function for the constant term by himself - this is contained in r60549. If he fits his data with ParamCount = 2 and basis functions sin(x) and sin(2x) only, but notices a poor agreement due to an offset he will automatically add a constant term to his model. |
|
|
Ok, this sounds reasonably. So let's require calling SetFitBasisFunc() for all indices in range 0 .. ParamCount-1. |
|
|
By the way: In tafitlib.pas, currently there is: TFitBaseFunc = record Func: TFitFunc; FuncName: String; end; TFitParam = record Func: TFitFunc; FuncName: String; Value: ArbFloat; Fixed: Boolean; end; a) As far as I remember, FuncName is used only in feCustom mode, so maybe it's not needed in TFitParam? b) Is there a need of splitting this into two records? In TFitSeries declaration, there are two arrays: FFitParams: TFitParamArray; FCustomFuncs: TFitBaseFuncArray; Both of them are assumed to have same length. So maybe this would be more simple: TFitParam = record Func: TFitFunc; CustomFunc: TFitFunc; CustomFuncName: String; Value: ArbFloat; Fixed: Boolean; end; And in TFitSeries.PrepareFitParams: if FFitEquation <> feCustom then FFitParams[i].Func := @FitBaseFunc_Poly else begin FFitParams[i].Func := FFitParams[i].CustomFunc; if FFitParams[i].Func = nil then exit; end; |
|
|
Of course. After fixing equation text to correctly deal with the constant term now, I think this issue can be considered to be resolved. |
|
|
Everything seems to be OK, I just wanted to make some note: currently, the fit demo program uses a FitBaseFunc_Const() function, while the TFitSeries.PrepareFitParams() method uses FitBaseFunc_Poly() in all cases for initialization, even for x^0, x^1 etc. I just wanted to note this to make sure that this is intentional. BTW: I could suggest better code: else begin if FFitParams[i].CustomFunc = nil then exit; FFitParams[i].Func := FFitParams[i].CustomFunc; end; |
|
|
And in tafuncseries.pas, there is an outdated comment: In case of custom fitting, the fit base functions become equal to the functions FCustomFuncs defined separately by the method SetFitBasisFunc(). The "FCustomFuncs" field no longer exists. |
|
|
Using FitBaseFunc_Const() is much more instructive to indicate that it refers to the constant term. Moreover, FitBaseFunc_Poly absolutely must be at index 0 of the function array because the array index is the exponent of the polynomial term, i.e. x^0 = 1. Or what do you think about using FitBaseFunc_Cos at index 0(cos(x*0) = 1) ;-) Applied the others. |
|
|
> about using FitBaseFunc_Cos at index 0(cos(x*0) = 1) ;-) I laughed loudly, point for you :D |
