View Issue Details

IDProjectCategoryView StatusLast Update
0035188LazarusTAChartpublic2019-04-10 16:17
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60589 
Target Version2.2Fixed in Version 
Summary0035188: TAChart: sorting issue in TListChartSource
DescriptionOpen the attached Reproduce application in IDE - there are two TListChartSource objects there.

1) select ListChartSource1, set YCount to 0, make Sorted enabled - as can be seen in DataPoints editor, data becomes sorted.

2) select ListChartSource2, set XCount to 0, make Sorted enabled - as can be seen in DataPoints editor, data does NOT become sorted.

The attached patch solves the problem.
TagsNo tags attached.
Fixed in Revision60856, 60918
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • Reproduce.zip (2,057 bytes)
  • patch.diff (969 bytes)
    Index: components/tachart/tasources.pas
    ===================================================================
    --- components/tachart/tasources.pas	(revision 60589)
    +++ components/tachart/tasources.pas	(working copy)
    @@ -863,9 +863,28 @@
         end;
     end;
     
    +function CompareDataItemY(AItem1, AItem2: Pointer): Integer;
    +var
    +  i: Integer;
    +  item1: PChartDataItem absolute AItem1;
    +  item2: PChartDataItem absolute AItem2;
    +begin
    +  Result := CompareFloat(item1^.Y, item2^.Y);
    +  if Result = 0 then
    +    for i := 0 to Min(High(item1^.YList), High(item2^.YList)) do begin
    +      Result := CompareFloat(item1^.YList[i], item2^.YList[i]);
    +      if Result <> 0 then
    +        exit;
    +    end;
    +end;
    +
     procedure TListChartSource.Sort;
     begin
    -  FData.Sort(@CompareDataItemX);
    +  if XCount > 0 then
    +    FData.Sort(@CompareDataItemX)
    +  else
    +  if YCount > 0 then
    +    FData.Sort(@CompareDataItemY);
     end;
     
     procedure TListChartSource.UpdateCachesAfterAdd(AX, AY: Double);
    
    patch.diff (969 bytes)
  • experiment.diff (3,234 bytes)
    Index: components/tachart/tacustomsource.pas
    ===================================================================
    --- components/tachart/tacustomsource.pas	(revision 60691)
    +++ components/tachart/tacustomsource.pas	(working copy)
    @@ -84,6 +84,13 @@
       TChartValueTextArray = array of TChartValueText;
     
       TChartDataItem = packed record
    +  private
    +    function GetXValues(AIndex: Integer): Double;
    +    function GetYValues(AIndex: Integer): Double;
    +    procedure SetXValues(AIndex: Integer; const AValue: Double);
    +    procedure SetYValues(AIndex: Integer; const AValue: Double);
    +    function GetXCount: Integer;
    +    function GetYCount: Integer;
       public
         X, Y: Double;
         Color: TChartColor;
    @@ -98,6 +105,11 @@
         procedure SetY(AValue: Double);
         procedure MultiplyY(ACoeff: Double);
         function Point: TDoublePoint; inline;
    +  public
    +    property XValues[AIndex: Integer]: Double read GetXValues write SetXValues;
    +    property YValues[AIndex: Integer]: Double read GetYValues write SetYValues;
    +    property XCount: Integer read GetXCount;
    +    property YCount: Integer read GetYCount;
       end;
       PChartDataItem = ^TChartDataItem;
     
    @@ -283,6 +295,9 @@
     uses
       Math, StrUtils, SysUtils, TAMath;
     
    +const
    +  SIndexOutOfRange = 'Index out of range.';
    +
     function CompareChartValueTextPtr(AItem1, AItem2: Pointer): Integer;
     begin
       Result := CompareValue(
    @@ -472,6 +487,16 @@
         Result := XList[AIndex - 1];
     end;
     
    +function TChartDataItem.GetXValues(AIndex: Integer): Double;
    +begin
    +  if (AIndex < 0) or (AIndex > Length(XList)) then
    +    raise EChartError.Create(SIndexOutOfRange);
    +  if AIndex = 0 then
    +    Result := X
    +  else
    +    Result := XList[AIndex - 1];
    +end;
    +
     function TChartDataItem.GetY(AIndex: Integer): Double;
     begin
       AIndex := EnsureRange(AIndex, 0, Length(YList));
    @@ -481,6 +506,16 @@
         Result := YList[AIndex - 1];
     end;
     
    +function TChartDataItem.GetYValues(AIndex: Integer): Double;
    +begin
    +  if (AIndex < 0) or (AIndex > Length(YList)) then
    +    raise EChartError.Create(SIndexOutOfRange);
    +  if AIndex = 0 then
    +    Result := Y
    +  else
    +    Result := YList[AIndex - 1];
    +end;
    +
     procedure TChartDataItem.MultiplyY(ACoeff: Double);
     var
       i: Integer;
    @@ -513,6 +548,16 @@
         XList[AIndex - 1] := AValue;
     end;
     
    +procedure TChartDataItem.SetXValues(AIndex: Integer; const AValue: Double);
    +begin
    +  if (AIndex < 0) or (AIndex > Length(XList)) then
    +    raise EChartError.Create(SIndexOutOfRange);
    +  if AIndex = 0 then
    +    X := AValue
    +  else
    +    XList[AIndex - 1] := AValue;
    +end;
    +
     procedure TChartDataItem.SetY(AValue: Double);
     var
       i: Integer;
    @@ -530,7 +575,27 @@
         YList[AIndex - 1] := AValue;
     end;
     
    +procedure TChartDataItem.SetYValues(AIndex: Integer; const AValue: Double);
    +begin
    +  if (AIndex < 0) or (AIndex > Length(YList)) then
    +    raise EChartError.Create(SIndexOutOfRange);
    +  if AIndex = 0 then
    +    Y := AValue
    +  else
    +    YList[AIndex - 1] := AValue;
    +end;
     
    +function TChartDataItem.GetXCount: Integer;
    +begin
    +  Result := Length(XList) + 1;
    +end;
    +
    +function TChartDataItem.GetYCount: Integer;
    +begin
    +  Result := Length(YList) + 1;
    +end;
    +
    +
     { TBasicChartSource }
     
     constructor TBasicChartSource.Create(AOwner: TComponent);
    
    experiment.diff (3,234 bytes)
  • Test.zip (2,867 bytes)
  • patch2.diff (817 bytes)
    Index: components/tachart/tasources.pas
    ===================================================================
    --- components/tachart/tasources.pas	(revision 60848)
    +++ components/tachart/tasources.pas	(working copy)
    @@ -743,6 +743,9 @@
       end;
     
     begin
    +  if Sorted then
    +    if IsNan(AValue) then
    +      raise EChartError.CreateFmt('X = NaN in sorted source %s', [NameOrClassName(Self)]);
       oldX := Item[AIndex]^.X;
       Result := AIndex;
       if IsEquivalent(oldX, AValue) then exit;
    @@ -749,8 +752,6 @@
       Item[AIndex]^.X := AValue;
       UpdateExtent;
       if Sorted then begin
    -    if IsNan(AValue) then
    -      raise EChartError.CreateFmt('X = NaN in sorted source %s', [NameOrClassName(Self)]);
         if AValue > oldX then
           while (Result < Count - 1) and (Item[Result + 1]^.X < AValue) do
             Inc(Result)
    
    patch2.diff (817 bytes)
  • revert.diff (969 bytes)
    Index: components/tachart/tasources.pas
    ===================================================================
    --- components/tachart/tasources.pas	(revision 60900)
    +++ components/tachart/tasources.pas	(working copy)
    @@ -854,28 +854,9 @@
         end;
     end;
     
    -function CompareDataItemY(AItem1, AItem2: Pointer): Integer;
    -var
    -  i: Integer;
    -  item1: PChartDataItem absolute AItem1;
    -  item2: PChartDataItem absolute AItem2;
    -begin
    -  Result := CompareFloat(item1^.Y, item2^.Y);
    -  if Result = 0 then
    -    for i := 0 to Min(High(item1^.YList), High(item2^.YList)) do begin
    -      Result := CompareFloat(item1^.YList[i], item2^.YList[i]);
    -      if Result <> 0 then
    -        exit;
    -    end;
    -end;
    -
     procedure TListChartSource.Sort;
     begin
    -  if XCount > 0 then
    -    FData.Sort(@CompareDataItemX)
    -  else
    -  if YCount > 0 then
    -    FData.Sort(@CompareDataItemY);
    +  FData.Sort(@CompareDataItemX);
     end;
     
     procedure TListChartSource.UpdateCachesAfterAdd(AX, AY: Double);
    
    revert.diff (969 bytes)
  • patch3.diff (1,450 bytes)
    Index: components/tachart/tasources.pas
    ===================================================================
    --- components/tachart/tasources.pas	(revision 60900)
    +++ components/tachart/tasources.pas	(working copy)
    @@ -34,6 +34,7 @@
         function NewItem: PChartDataItem;
         procedure SetDataPoints(AValue: TStrings);
         procedure SetSorted(AValue: Boolean);
    +    procedure SetXListNoSort(AIndex: Integer; const AXList: array of Double);
         procedure UpdateCachesAfterAdd(AX, AY: Double);
       protected
         function GetCount: Integer; override;
    @@ -582,7 +583,7 @@
         for i := 0 to ASource.Count - 1 do
           with ASource[i]^ do begin
             AddAt(FData.Count, X, Y, Text, Color);
    -        SetXList(FData.Count - 1, XList);
    +        SetXListNoSort(FData.Count - 1, XList);
             SetYList(FData.Count - 1, YList);
           end;
         if Sorted and not ASource.IsSorted then Sort;
    @@ -719,7 +720,7 @@
       Notify;
     end;
     
    -procedure TListChartSource.SetXList(
    +procedure TListChartSource.SetXListNoSort(
       AIndex: Integer; const AXList: array of Double);
     var
       i: Integer;
    @@ -730,6 +731,16 @@
       FXListExtentIsValid := false;
     end;
     
    +procedure TListChartSource.SetXList(
    +  AIndex: Integer; const AXList: array of Double);
    +begin
    +  SetXListNoSort(AIndex, AXList);
    +  if Sorted then begin
    +    Sort;
    +    Notify;
    +  end;
    +end;
    +
     function TListChartSource.SetXValue(AIndex: Integer; AValue: Double): Integer;
     var
       oldX: Double;
    
    patch3.diff (1,450 bytes)

Activities

Marcin Wiazowski

2019-03-05 01:54

reporter  

Reproduce.zip (2,057 bytes)

Marcin Wiazowski

2019-03-05 01:55

reporter  

patch.diff (969 bytes)
Index: components/tachart/tasources.pas
===================================================================
--- components/tachart/tasources.pas	(revision 60589)
+++ components/tachart/tasources.pas	(working copy)
@@ -863,9 +863,28 @@
     end;
 end;
 
+function CompareDataItemY(AItem1, AItem2: Pointer): Integer;
+var
+  i: Integer;
+  item1: PChartDataItem absolute AItem1;
+  item2: PChartDataItem absolute AItem2;
+begin
+  Result := CompareFloat(item1^.Y, item2^.Y);
+  if Result = 0 then
+    for i := 0 to Min(High(item1^.YList), High(item2^.YList)) do begin
+      Result := CompareFloat(item1^.YList[i], item2^.YList[i]);
+      if Result <> 0 then
+        exit;
+    end;
+end;
+
 procedure TListChartSource.Sort;
 begin
-  FData.Sort(@CompareDataItemX);
+  if XCount > 0 then
+    FData.Sort(@CompareDataItemX)
+  else
+  if YCount > 0 then
+    FData.Sort(@CompareDataItemY);
 end;
 
 procedure TListChartSource.UpdateCachesAfterAdd(AX, AY: Double);
patch.diff (969 bytes)

wp

2019-03-10 11:34

developer   ~0114764

Last edited: 2019-03-10 11:35

View 2 revisions

Your patch in 0035210 demonstrates to me that follower actions are required if the user is allowed to think that having XCount=0 or YCount=0 is a meaningful use case. It is not, except for the purpose of axis labeling, where it has drawbacks too as I noted elsewhere.

Therefore I reject this patch. TAChart should not contain any code which makes XCount=0 or YCount=0 valid options in any sense. In the long term, this case should probably be prohibited by raising an exception even for the DataPoints editor.

Marcin Wiazowski

2019-03-10 21:53

reporter   ~0114781

The sample application, that I attached to 0035210, might in fact suggest, that the problem is with XCount = 0 or YCount = 0, but the problem is elsewhere - so I made some additional explanations there.


I think you made a good decision to allow YCount be 0 (and, similarly, XCount). The next useful example of YCount = 0 usage is TColorMapSeries, which has ColorSource property, using only X and Color information. In fact, there are still some issues with XCount/YCount = 0, I found two (well, quite simple) problems, so I'm going to provide a patch. It's not my decision at all, but you made some efforts to implement YCount = 0, which is at least a bit useful. Wouldn't be abandoning this feature just wasting the work now, when the functionality is in fact almost ready?

If you want to abandon the feature now, please just let me know.

If you are still not sure, I'll provide a patch for the problems that I found, so you will be able to see if the feature is still problematic.

wp

2019-03-11 13:03

developer   ~0114786

> The next useful example of YCount = 0 usage is TColorMapSeries, which has
> ColorSource property, using only X and Color information.

To convince me provide a demo. But a linear gradient in x direction by a color source in which the dummy y value (see http://wiki.lazarus.freepascal.org/TAChart_Tutorial:_ColorMapSeries,_Zooming#Setting_up_the_color_map) can be omitted due to YCount=0 will not convince me - it's just the same as dropping the x or y column in the data point editor. I doubt that the two issues that you indicate will be the last ones if XCount/YCount=0 will be allowed.

wp

2019-03-13 18:17

developer   ~0114820

Maybe I should better explain why XCount/YCount = 0 is not an option for me. Here is the declaration of TChartDataItem which is the basic data point record provided by the chart series:

  type
    TChartDataItem = packed record
    public
      X, Y: Double;
      Color: TChartColor;
      Text: String;
      XList: TDoubleDynArray;
      YList: TDoubleDynArray;
      ...
    end;

I don't know why it was made this way. Maybe it's just historical: It began with X and Y elements only. Then somebody thought it might be good to provide several y values by adding the YList array. The most compatible way is to leave the Y element untouched and add the additional y values as an extra array. And later somebody else (it was me this time) thought: why don't we provide several x values? And I followed the same idea.

Whatever the reason is - we have it this way, and a lot of code depends on it.

With this declaration there will always be elements X and Y, even if XCount and YCount are defined as being zero, and these elements have values, defined or undefined, and can be used when XCount and YCount are not checked. Even if X and Y are initialized with NaN or something similar, and all critical checks for XCount > 0 and YCount > 0 are made, the user can always ask himself: What a strange library which contains variables that do not exist.

If we want to allow for XCount = 0 and/or YCount = 0, then the elements X and Y of the TChartDataItem record must be removed, and all data values must be stuffed into the XList and YList arrays. Besides a small slow-down of data access (reading/writing ChartDataitem.XList[0] is slower that reading/writing the element X directly), the main disadvantage, however, is that it breaks a lot of code within TAChart and also code written by users. So far, the y values are given as
- Y -- "normal" y value
- YList[0] -- first additional y value
- YList[1] -- second additional y value, etc.
TChartSeries has a method AddXY(X, Y, YList)...

Afterwards, they would be given as
- YList[0] -- normal y value
- YList[1] -- first additional y value
- YList[2] -- second addtional y value etc.

I am not strictly against code-braking changes, but there must be good arguments for them. Just being able to drop the Y column in the DataPoints editor for defining x axis labels or colors for a ColorMapSeries, is not a good argument. There are probably other ways to achieve this (e.g. add a property "Usage" to the TListChartSource, TSourceUsage = (suDataPoint, suXLabel, suYLabel, suColorMap), which controls the visible columns in the DataPoints editor).

Having XList and YList only certainly would be much clearer to understand than the current mix of X and XList as well as Y and YList. But is this argument good enough the justify breaking user code? I don't think so.

Marcin Wiazowski

2019-03-16 01:56

reporter  

experiment.diff (3,234 bytes)
Index: components/tachart/tacustomsource.pas
===================================================================
--- components/tachart/tacustomsource.pas	(revision 60691)
+++ components/tachart/tacustomsource.pas	(working copy)
@@ -84,6 +84,13 @@
   TChartValueTextArray = array of TChartValueText;
 
   TChartDataItem = packed record
+  private
+    function GetXValues(AIndex: Integer): Double;
+    function GetYValues(AIndex: Integer): Double;
+    procedure SetXValues(AIndex: Integer; const AValue: Double);
+    procedure SetYValues(AIndex: Integer; const AValue: Double);
+    function GetXCount: Integer;
+    function GetYCount: Integer;
   public
     X, Y: Double;
     Color: TChartColor;
@@ -98,6 +105,11 @@
     procedure SetY(AValue: Double);
     procedure MultiplyY(ACoeff: Double);
     function Point: TDoublePoint; inline;
+  public
+    property XValues[AIndex: Integer]: Double read GetXValues write SetXValues;
+    property YValues[AIndex: Integer]: Double read GetYValues write SetYValues;
+    property XCount: Integer read GetXCount;
+    property YCount: Integer read GetYCount;
   end;
   PChartDataItem = ^TChartDataItem;
 
@@ -283,6 +295,9 @@
 uses
   Math, StrUtils, SysUtils, TAMath;
 
+const
+  SIndexOutOfRange = 'Index out of range.';
+
 function CompareChartValueTextPtr(AItem1, AItem2: Pointer): Integer;
 begin
   Result := CompareValue(
@@ -472,6 +487,16 @@
     Result := XList[AIndex - 1];
 end;
 
+function TChartDataItem.GetXValues(AIndex: Integer): Double;
+begin
+  if (AIndex < 0) or (AIndex > Length(XList)) then
+    raise EChartError.Create(SIndexOutOfRange);
+  if AIndex = 0 then
+    Result := X
+  else
+    Result := XList[AIndex - 1];
+end;
+
 function TChartDataItem.GetY(AIndex: Integer): Double;
 begin
   AIndex := EnsureRange(AIndex, 0, Length(YList));
@@ -481,6 +506,16 @@
     Result := YList[AIndex - 1];
 end;
 
+function TChartDataItem.GetYValues(AIndex: Integer): Double;
+begin
+  if (AIndex < 0) or (AIndex > Length(YList)) then
+    raise EChartError.Create(SIndexOutOfRange);
+  if AIndex = 0 then
+    Result := Y
+  else
+    Result := YList[AIndex - 1];
+end;
+
 procedure TChartDataItem.MultiplyY(ACoeff: Double);
 var
   i: Integer;
@@ -513,6 +548,16 @@
     XList[AIndex - 1] := AValue;
 end;
 
+procedure TChartDataItem.SetXValues(AIndex: Integer; const AValue: Double);
+begin
+  if (AIndex < 0) or (AIndex > Length(XList)) then
+    raise EChartError.Create(SIndexOutOfRange);
+  if AIndex = 0 then
+    X := AValue
+  else
+    XList[AIndex - 1] := AValue;
+end;
+
 procedure TChartDataItem.SetY(AValue: Double);
 var
   i: Integer;
@@ -530,7 +575,27 @@
     YList[AIndex - 1] := AValue;
 end;
 
+procedure TChartDataItem.SetYValues(AIndex: Integer; const AValue: Double);
+begin
+  if (AIndex < 0) or (AIndex > Length(YList)) then
+    raise EChartError.Create(SIndexOutOfRange);
+  if AIndex = 0 then
+    Y := AValue
+  else
+    YList[AIndex - 1] := AValue;
+end;
 
+function TChartDataItem.GetXCount: Integer;
+begin
+  Result := Length(XList) + 1;
+end;
+
+function TChartDataItem.GetYCount: Integer;
+begin
+  Result := Length(YList) + 1;
+end;
+
+
 { TBasicChartSource }
 
 constructor TBasicChartSource.Create(AOwner: TComponent);
experiment.diff (3,234 bytes)

Marcin Wiazowski

2019-03-16 02:02

reporter   ~0114853

First, sorry about not responding to your notices for last few days. This was due to my personal reasons.


You made a comprehensive review of the TChartDataItem above, so I'll only add a few words. Changing the record by removing X and Y fields and using only XList and YList sounds like a nightmare for me:
- requires rewriting TAChart's code in many places,
- may require additional validating of XList / YList's length, even when accessing the first item,
- breaks compatibility with the already existing users' code,
and all of this just to achieve a more consistent, "better look" of the record... I think that such a change could be considered only if it would be the only possible way for further chart's development.


But I think that you look at the problem too much from the TAChart insider's point of view. From the user's point of view, the things may look rather like:
- having XCount / YCount set to some value - in particular to zero - is a high-level feature, while TChartDataItem is just an internal, low-level implementation detail,
- most of users will never see or use TChartDataItem directly, because working on series' methods, like AddXY or AddVector, along with chart tools, is sufficient in most cases.


I have also some another observation:
- in TChartAxis.Range, Min is valid only when UseMin is True,
- in TFuncSeries.Extent, XMin is valid only when UseXMin is True,
- in TChartStyle, Brush is valid only when UseBrush is True, and similarly Font and Pen,
- so I can't see anything strange in TChartDataItem.Y being valid only when YCount > 0.


I'm not sure if I understood the idea with TSourceUsage correctly, but having two variables (i.e. YCount and Usage:TSourceUsage) to describe the same thing may be rather confusing than helpful...



But I think, that there is a third way - where the user may both get an intuitive, zero-based access to TChartDataItem's coordinates, and also may benefit from having XCount / YCount equal to zero (where the advantage is, in particular, in LFM streaming). And, what's more, full compatibility is still retained.

Please take a look at the attached experiment.diff. The TChartDataItem record has been enriched by adding four properties:

  TChartDataItem = packed record
  private
    function GetXValues(AIndex: Integer): Double;
    function GetYValues(AIndex: Integer): Double;
    procedure SetXValues(AIndex: Integer; const AValue: Double);
    procedure SetYValues(AIndex: Integer; const AValue: Double);
    function GetXCount: Integer;
    function GetYCount: Integer;
  public
    ...
  public
    property XValues[AIndex: Integer]: Double read GetXValues write SetXValues;
    property YValues[AIndex: Integer]: Double read GetYValues write SetYValues;
    property XCount: Integer read GetXCount;
    property YCount: Integer read GetYCount;
  end;

Now the user has a full choice: he can still use lower-level X / Y / XList / YList fields, or rather higher-level XValues[] / YValues[] / XCount / YCount properties. The newly introduced XValues[] and YValues[] are zero-based, and they verify the requested index (and raise an exception in case of out-of-bounds value). When using XValues[] and YValues[], even having TListChartSource with XCount or YCount = 0 seems to be natural.

wp

2019-03-16 17:25

developer   ~0114871

Isn't this the same as what TChartDataItem.GetX and .GetY are doing already?

And this patch makes the situation even more confusing because YCount can be set to zero in the chart source, but the getter of the new equally-named property of the TChartDataItem never returns 0.

What are the advantages in having XCount/YCount=0? In normal usage of TAChart data are not provided by a ListSource at designtime, i.e. data points are not streamed to LFM. In the rare case when somebody uses the data point editor to define designtime data I don't see a real disadvantage when an unneeded x or y value is written to LFM. And - I already said it - even if the y values in a the ListChartSource to be used for x axis labels appear obsolete you will be glad to have the duplicated x value here once you decide to rotate the series axes.

So far I cannot imagine an urgent necessity for XCount/YCount=0 which would justify a change of the inner-most parts of TAChart.

Marcin Wiazowski

2019-03-16 20:49

reporter   ~0114878

> Isn't this the same as what TChartDataItem.GetX and .GetY are doing already?

In fact yes, but a range validation has been added, and the property is accessed in an array manner, which may be just more intuitive for some users.



> And this patch makes the situation even more confusing because YCount can be set to zero in the chart source, but the getter of the new equally-named property of the TChartDataItem never returns 0.

Yes, that's true - this was just an experiment, in which the record's YCount property returns the highest number of elements, that will not raise an out-of-bounds exception. There are at least two other solutions:

  TChartDataItem = packed record
    ...
    X, Y: Double;
    UseX, UseY: Boolean; <==== ADDED
    ...

  function TChartDataItem.GetYValues(AIndex: Integer): Double;
  begin
    if (not UseY) or (AIndex < 0) or (AIndex > Length(YList)) then
      raise EChartError.Create(SIndexOutOfRange);
    ...
  end;

  function TChartDataItem.GetYCount: Integer;
  begin
    if UseY then
      Result := Length(YList) + 1
    else
      Result := 0;
  end;

But probably the best solution would be just to remove record's XCount and YCount properties, so the user will use source's XCount and YCount values.



> you will be glad to have the duplicated x value here once you decide to rotate the series axes.

That's all true. But, after all, it's always the user's responsibility to use such ListChartSource's settings, that will be valid for his specific case.



I don't know when and why a possibility of setting YCount to 0 has been introduced. However, I can see three advantages of the current situation:

1) as discussed many times: LFM streaming when ListChartSource is used for axis labeling, or as a color source in TColorMapSeries,

2) much faster extent calculation when YCount = 0 and Sorted = True - it's enough to read X value of the first and the last data point - there is no need to iterate through all the data points to check their Y values,

3) the user may implement his own class of series, where Y value is constant (and is set by using some series' property), so data source does not need to hold Y values.



> I cannot imagine an urgent necessity for XCount/YCount=0 which would justify a change of the inner-most parts of TAChart.

In fact, there is no necessity for YCount = 0. It's just a feature, that may be sometimes useful. But there is also no need to change inner-most parts of TAChart - this already is implemented and works. In fact, there are some issues, but they can be fixed by making some simple changes, like adding "if YCount = 0 then exit" in TListChartSource.SetYValue().

wp

2019-03-16 23:24

developer   ~0114882

I must say that I am a bit lost now:


> I don't know when and why a possibility of setting YCount to 0 has been introduced.

Wasn't it you who absolutely wanted to set YCount = 0?

---------

> much faster extent calculation when YCount = 0 and Sorted = True - it's
> enough to read X value of the first and the last data point - there is no
> need to iterate through all the data points to check their Y values,

Why does YCount have to be 0 to read the lowest and highest x value from a chartsource that is sorted in x? And how can you check y values when there are none because YCount is zero?

--------

> the user may implement his own class of series, where Y value is constant (and
> is set by using some series' property), so data source does not need to hold Y
> values.

We already have that: TConstantLine, and it does not even need a chart source...

Marcin Wiazowski

2019-03-17 00:18

reporter   ~0114884

> Wasn't it you who absolutely wanted to set YCount = 0?

Not exactly. Even in tasources.pas from revision r50537 (Lazarus 1.6), we can find "if FSource.YCount > 0 then" phrases, so YCount can be zero at least since 3 years. I don't know if this fully worked, but when providing my patches, I was aware of these "if FSource.YCount > 0 then" statements, so I realized that YCount can be zero, and tried to follow this convention. And this way I started to look like a zealous supporter of the zero-YCount idea. But this wasn't in fact my idea, this has been implemented some years ago.



> Why does YCount have to be 0 to read the lowest and highest x value from a chartsource that is sorted in x? And how can you check y values when there are none because YCount is zero?

It seems that I wasn't clear enough. When calculating the extent for the XCount = YCount = 1 case, we need to find minimum and maximum X value, and also minimum and maximum Y value. When Sorted = True, finding minimum and maximum X value is trivial - first data point holds the minimum X value, and last data point holds the maximum X value. But to find minimum and maximum Y value, we need to enumerate, and check, all the data points.

But when we have YCount = 0, the extent will be -INF .. +INF for Y values (just like in an empty extent), and X[first] .. X[last] for X values. So we don't need to enumerate all the data points to calculate the extent in this case - it's enough to read X[first] and X[last].



> We already have that: TConstantLine, and it does not even need a chart source...

Yes, I know. I was rather thinking about something that can be seen in 0034819, at the ThisBugCanBeUseful.png image attached there. Each event kind (Note, Success, Failure) has its own Y, so data source needs to store only X values (time) and labels.

wp

2019-03-19 00:56

developer   ~0114917

Last edited: 2019-03-19 10:08

View 2 revisions

>> Wasn't it you who absolutely wanted to set YCount = 0?

> Not exactly. Even in tasources.pas from revision r50537 (Lazarus 1.6), we can find "if FSource.YCount > 0 then" phrases, so YCount
> can be zero at least since 3 years. I don't know if this fully worked, but when providing my patches, I was aware of these "if
> FSource.YCount > 0 then" statements, so I realized that YCount can be zero, and tried to follow this convention. And this way I
> started to look like a zealous supporter of the zero-YCount idea. But this wasn't in fact my idea, this has been implemented some
> years ago.

There's certainly no plan behind these lines (except for protecting from obvious user errors).

---------------

>> Why does YCount have to be 0 to read the lowest and highest x value from a chartsource that is sorted in x? And how can you check
>> y values when there are none because YCount is zero?

> It seems that I wasn't clear enough. When calculating the extent for the XCount = YCount = 1 case, we need to find minimum and
> maximum X value, and also minimum and maximum Y value. When Sorted = True, finding minimum and maximum X value is trivial - first
> data point holds the minimum X value, and last data point holds the maximum X value. But to find minimum and maximum Y value, we
> need to enumerate, and check, all the data points.
>
> But when we have YCount = 0, the extent will be -INF .. +INF for Y values (just like in an empty extent), and X[first] .. X[last]
> for X values. So we don't need to enumerate all the data points to calculate the extent in this case - it's enough to read
> X[first] and X[last].

I still don't get it. Are you talking of the extent for XCount=YCount=1? Then you must sort twice, don't you? Or are you talking of YCount = 0? After setting Sorted to true x values will be sorted and you can get the x extent from X[first] and X[last] always, whether YCount is zero or not.

---------------

>> We already have that: TConstantLine, and it does not even need a chart source...

> Yes, I know. I was rather thinking about something that can be seen in 0034819, at the ThisBugCanBeUseful.png image attached
> there. Each event kind (Note, Success, Failure) has its own Y, so data source needs to store only X values (time) and labels.

Can't see the advantage of having the Y somewhere else than using the one which is already there. And when the Y is taken from the index into an array of strings then the strings are not in the TChartDataItem where they are supposed to be.

Marcin Wiazowski

2019-03-21 00:03

reporter  

Test.zip (2,867 bytes)

Marcin Wiazowski

2019-03-21 00:05

reporter   ~0114952

About extents: I was wrong. Currently, TListChartSource's extent calculation is in fact delegated to TCustomChartSource.BasicExtent():

  function TCustomChartSource.BasicExtent: TDoubleRect;
  begin
    ...

    if XCount > 0 then begin
      if HasXErrorBars then
        for i := 0 to Count - 1 do begin
          GetXErrorBarLimits(i, vhi, vlo);
          UpdateMinMax(vhi, FExtent.a.X, FExtent.b.X);
          UpdateMinMax(vlo, FExtent.a.X, FExtent.b.X);
        end
      else
        for i:=0 to Count - 1 do
          UpdateMinMax(Item[i]^.X, FExtent.a.X, FExtent.b.X);
    end;

    if YCount > 0 then begin
      if HasYErrorBars then
        for i := 0 to Count - 1 do begin
          GetYErrorBarLimits(i, vhi, vlo);
          UpdateMinMax(vhi, FExtent.a.Y, FExtent.b.Y);
          UpdateMinMax(vlo, FExtent.a.Y, FExtent.b.Y);
        end
      else
        for i:=0 to Count - 1 do
          UpdateMinMax(Item[i]^.Y, FExtent.a.Y, FExtent.b.Y);
    end;

    ...
  end;

For XCount = 1 and YCount = 1, both "for" loops must be executed. But if we know, that we don't need X or Y values, we can set source's XCount or YCount to 0, so one "for" loop can be omitted.




> There's certainly no plan behind these lines (except for protecting from obvious user errors).

Indeed, you are right. Now I can also see it. But few months ago, when I saw these lines and I was also able to set YCount to 0 in Object Inspector, I just logically assumed, that YCount = 0 is handled. From the other side, adding one "if YCount > 0 then" phrase to TCustomChartSource's implementation made this really working. There are still some simple issues in a higher-level TListChartSource (which extends TCustomChartSource's functionality), but can be also fixed easily. One of them is sorting, and the patch attached here solves it. A second is in TListChartSource.SetXValue/SetYValue - additional checks for XCount/YCount = 0 are needed to avoid improper updates of the cached extent. And it seems to be all...




> Can't see the advantage of having the Y somewhere else than using the one which is already there. And when the Y is taken from the index into an array of strings then the strings are not in the TChartDataItem where they are supposed to be.

Let's make it clear: when source is loaded into memory, it uses TChartDataItem memory records, that contain Y fields - so using Y located elsewhere does not make much sense. However, when source is stored in a stream, it doesn't need to store Y values if they are useless.

I'm attaching a test application, that shows the idea of using custom series, requiring only X values stored in the source, so source's YCount is set to 0. Interestingly, thanks to the fact, that - after loading into memory - TChartDataItem.Y values are available - they are used. I'm fully aware of fact, that the attached example is a total overkill for such a simple case - but it's only an example, that some user-defined series class may require only X values.

wp

2019-03-21 00:30

developer   ~0114953

Not convinced I applied it nevertheless to see where it goes to. Please provide also patches for the remaining issues related to XCount=0, YCount=0.

Marcin Wiazowski

2019-04-06 03:30

reporter  

patch2.diff (817 bytes)
Index: components/tachart/tasources.pas
===================================================================
--- components/tachart/tasources.pas	(revision 60848)
+++ components/tachart/tasources.pas	(working copy)
@@ -743,6 +743,9 @@
   end;
 
 begin
+  if Sorted then
+    if IsNan(AValue) then
+      raise EChartError.CreateFmt('X = NaN in sorted source %s', [NameOrClassName(Self)]);
   oldX := Item[AIndex]^.X;
   Result := AIndex;
   if IsEquivalent(oldX, AValue) then exit;
@@ -749,8 +752,6 @@
   Item[AIndex]^.X := AValue;
   UpdateExtent;
   if Sorted then begin
-    if IsNan(AValue) then
-      raise EChartError.CreateFmt('X = NaN in sorted source %s', [NameOrClassName(Self)]);
     if AValue > oldX then
       while (Result < Count - 1) and (Item[Result + 1]^.X < AValue) do
         Inc(Result)
patch2.diff (817 bytes)

Marcin Wiazowski

2019-04-06 03:31

reporter   ~0115260

To make sorting working fully properly (not only when XCount or YCount = 0), some additional fixes are needed.

Before that, I'm attaching patch2.diff to solve a problem in TListChartSource.SetXValue(): when source is sorted, X = NaN should not be set, so exception is raised - but it's currently raised AFTER X = NaN is already written do the source. This doesn't make sense - the attached patch prevents this.

wp

2019-04-06 10:25

developer   ~0115261

Applied. Thanks.

Marcin Wiazowski

2019-04-08 19:06

reporter   ~0115331

I'd like to ask you for your advice.



TCustomChartSource introduces only a dummy sorting implementation:

  function TCustomChartSource.IsSorted: Boolean;
  begin
    Result := false;
  end;

Even though, TCustomChartSource calls IsSorted(), in particular in TCustomChartSource.FindBounds() - this is because IsSorted() can be overridden in descendant classes (as it is in TListChartSource).

Observation 1: when TCustomChartSource.FindBounds() calls IsSorted(), it wants to check, if data source is sorted BY X VALUES (this isn't in fact surprising).


Observation 2: TListChartSource.Add() reads Sorted property (which is equivalent to calling IsSorted() method) and also assumes, that Sorted means "sorted BY X VALUES".


But currently - due to change in TListChartSource.Sort(), that I suggested - data can be sorted either by X, or by Y.


----


So I can see the following solutions:


1) TListChartSource.Sort() can be reverted to previous state, where data can be sorted only by X. This automatically solves all the issues like Observation 1 and 2.


2) Sorting is just abstract in TCustomChartSource, and there exist potentially unlimited number of sorting algorithms, that can be implemented in TCustomChartSource's descendants; one of possible sorting algorithms is implemented in TListChartSource.

So I could add:

  function TCustomChartSource.IsSortedByX: Boolean;
  begin
    Result := false;
  end;

  function TCustomChartSource.IsSortedByY: Boolean;
  begin
    Result := false;
  end;

which could be then overridden in TListChartSource():

  function TListChartSource.IsSortedByX: Boolean;
  begin
    Result := IsSorted and (XCount > 0);
  end;

  function TListChartSource.IsSortedByY: Boolean;
  begin
    Result := IsSorted and (XCount = 0) and (YCount > 0);
  end;


Thanks to this change, TCustomChartSource.FindBounds() would be able to call IsSortedByX(), TListChartSource.Add() would be able to behave correctly for both IsSortedByX() and IsSortedByY(), and so on.


I can implement this easily, but I just don't want to force ideas like that. What do you prefer?

wp

2019-04-09 11:16

developer   ~0115340

Being a dumb TAChart user, not knowing about the background and the necessity to have XCount = 0, what could I think when I see a public IsSortedByY method? - Oh, I can sort my source by y. But why not by the label text? And how should I achieve sorting by y? There's no parameter to select y sorting in the Sort method. What a sh...!

Is sorting by y of any practical relevance? According to your code, this would happen for a source for which XCount has been set to 0. The old question: Is this of any practical importance? OK - data point editor, but any sorting feature could be implemente there. Or: somewhere you posted a series having YCount = 0 (which certainly could be rewritten to the case XCount=0), also not very convincing for me because that same could be achieved easily with a YCount <> 0 source as well.

So, I am still not convinced that this is necessary. Why don't we put that entire XCount=0/YCount=0 topic aside and wait until a real need for it comes up? We can keep the current code, I think it's not harmful, and it won't be the first incomplete idea within TAChart.

Marcin Wiazowski

2019-04-09 23:02

reporter  

revert.diff (969 bytes)
Index: components/tachart/tasources.pas
===================================================================
--- components/tachart/tasources.pas	(revision 60900)
+++ components/tachart/tasources.pas	(working copy)
@@ -854,28 +854,9 @@
     end;
 end;
 
-function CompareDataItemY(AItem1, AItem2: Pointer): Integer;
-var
-  i: Integer;
-  item1: PChartDataItem absolute AItem1;
-  item2: PChartDataItem absolute AItem2;
-begin
-  Result := CompareFloat(item1^.Y, item2^.Y);
-  if Result = 0 then
-    for i := 0 to Min(High(item1^.YList), High(item2^.YList)) do begin
-      Result := CompareFloat(item1^.YList[i], item2^.YList[i]);
-      if Result <> 0 then
-        exit;
-    end;
-end;
-
 procedure TListChartSource.Sort;
 begin
-  if XCount > 0 then
-    FData.Sort(@CompareDataItemX)
-  else
-  if YCount > 0 then
-    FData.Sort(@CompareDataItemY);
+  FData.Sort(@CompareDataItemX);
 end;
 
 procedure TListChartSource.UpdateCachesAfterAdd(AX, AY: Double);
revert.diff (969 bytes)

Marcin Wiazowski

2019-04-09 23:02

reporter  

patch3.diff (1,450 bytes)
Index: components/tachart/tasources.pas
===================================================================
--- components/tachart/tasources.pas	(revision 60900)
+++ components/tachart/tasources.pas	(working copy)
@@ -34,6 +34,7 @@
     function NewItem: PChartDataItem;
     procedure SetDataPoints(AValue: TStrings);
     procedure SetSorted(AValue: Boolean);
+    procedure SetXListNoSort(AIndex: Integer; const AXList: array of Double);
     procedure UpdateCachesAfterAdd(AX, AY: Double);
   protected
     function GetCount: Integer; override;
@@ -582,7 +583,7 @@
     for i := 0 to ASource.Count - 1 do
       with ASource[i]^ do begin
         AddAt(FData.Count, X, Y, Text, Color);
-        SetXList(FData.Count - 1, XList);
+        SetXListNoSort(FData.Count - 1, XList);
         SetYList(FData.Count - 1, YList);
       end;
     if Sorted and not ASource.IsSorted then Sort;
@@ -719,7 +720,7 @@
   Notify;
 end;
 
-procedure TListChartSource.SetXList(
+procedure TListChartSource.SetXListNoSort(
   AIndex: Integer; const AXList: array of Double);
 var
   i: Integer;
@@ -730,6 +731,16 @@
   FXListExtentIsValid := false;
 end;
 
+procedure TListChartSource.SetXList(
+  AIndex: Integer; const AXList: array of Double);
+begin
+  SetXListNoSort(AIndex, AXList);
+  if Sorted then begin
+    Sort;
+    Notify;
+  end;
+end;
+
 function TListChartSource.SetXValue(AIndex: Integer; AValue: Double): Integer;
 var
   oldX: Double;
patch3.diff (1,450 bytes)

Marcin Wiazowski

2019-04-09 23:03

reporter   ~0115364

> We can keep the current code, I think it's not harmful

I'm afraid, that I introduced an ugly bug... I mean: sorting by Y must be implemented either fully, or not at all. Currently, all the TListChartSource's and TCustomChartSource's code blindly assumes, that Sorted - when being set - guarantees, that data is sorted by X. But - due to change that I introduced - this is no longer guaranteed... So this specific change in TListChartSource.Sort() must be reverted - I'm attaching revert.diff for this purpose.



There is also another sorting problem (let's assume, that TListChartSource.Sort() sorts only by X, again): CompareDataItemX() function compares X fields, but - when they are equal - starts to compare XList fields. This means, that:

- changing or adding some X value needs resorting - which is implemented in TListChartSource.SetXValue() and TListChartSource.Add()

- changing or adding some XList value ALSO needs resorting - which is NOT currently implemented in TListChartSource.SetXList().

So I'm attaching patch3.diff, that fixes this problem. To avoid horrible slowdowns in TListChartSource.CopyFrom(), the patch introduces an internal SetXListNoSort() method.

wp

2019-04-10 09:53

developer   ~0115380

Reverted r60740.

wp

2019-04-10 10:02

developer   ~0115382

> There is also another sorting problem (let's assume, that
> TListChartSource.Sort() sorts only by X, again): CompareDataItemX() function
> compares X fields, but - when they are equal - starts to compare XList fields.

I probably introduced this without knowing where the XList would lead to. ATM, the XList values can have any meaning, depending on the series. Imagine two data points, one with X = 1 and XList[0] = 1, and the other one with X = 1 and XList[0] = 2. Is the second one "greater" than the first one? It depends...

- Used in a TFieldSeries, the XList[0] would be considered to be the x coordinate of the arrow direction vector and thus be added to the X value, i.e. the second data point would be the greater one.

- But if the source would be used by a series which gets its negative error bar value from XList[0] then XList[0] would be subtracted from X, and the second data point would be the smaller one.

- Or consider a new 3D contour plot series which interprets the X and XList[0] as the x/y coordinates as a point on the data plane and Y as the elevation above this plane, then X and XList[0] have nothing in common and both data points would be equal.

Therefore, I think that XList should not be considered at all when comparing data points.

Marcin Wiazowski

2019-04-10 13:57

reporter   ~0115388

Indeed, you are right.

Therefore, Sort() should ignore XList, and patch3.diff should NOT be applied.

wp

2019-04-10 14:29

developer   ~0115389

To summarize: Although the original idea of the reporter was not applied two changes were made to improve the already existing sorting by x.

Marcin Wiazowski

2019-04-10 16:17

reporter   ~0115391

Thanks for the constructive discussion above.

Issue History

Date Modified Username Field Change
2019-03-05 01:54 Marcin Wiazowski New Issue
2019-03-05 01:54 Marcin Wiazowski File Added: Reproduce.zip
2019-03-05 01:55 Marcin Wiazowski File Added: patch.diff
2019-03-06 18:04 wp Assigned To => wp
2019-03-06 18:04 wp Status new => assigned
2019-03-10 11:34 wp Note Added: 0114764
2019-03-10 11:35 wp LazTarget => -
2019-03-10 11:35 wp Status assigned => resolved
2019-03-10 11:35 wp Resolution open => won't fix
2019-03-10 11:35 wp Note Edited: 0114764 View Revisions
2019-03-10 21:53 Marcin Wiazowski Note Added: 0114781
2019-03-10 21:53 Marcin Wiazowski Status resolved => assigned
2019-03-10 21:53 Marcin Wiazowski Resolution won't fix => reopened
2019-03-11 13:03 wp Note Added: 0114786
2019-03-13 18:17 wp Note Added: 0114820
2019-03-15 10:47 wp Status assigned => resolved
2019-03-15 10:47 wp Resolution reopened => won't fix
2019-03-16 01:56 Marcin Wiazowski File Added: experiment.diff
2019-03-16 02:02 Marcin Wiazowski Note Added: 0114853
2019-03-16 02:02 Marcin Wiazowski Status resolved => assigned
2019-03-16 02:02 Marcin Wiazowski Resolution won't fix => reopened
2019-03-16 17:25 wp Note Added: 0114871
2019-03-16 20:49 Marcin Wiazowski Note Added: 0114878
2019-03-16 23:24 wp Note Added: 0114882
2019-03-17 00:18 Marcin Wiazowski Note Added: 0114884
2019-03-19 00:56 wp Note Added: 0114917
2019-03-19 10:08 wp Note Edited: 0114917 View Revisions
2019-03-21 00:03 Marcin Wiazowski File Added: Test.zip
2019-03-21 00:05 Marcin Wiazowski Note Added: 0114952
2019-03-21 00:30 wp Fixed in Revision => 60740
2019-03-21 00:30 wp LazTarget - => 2.2
2019-03-21 00:30 wp Note Added: 0114953
2019-03-21 00:30 wp Status assigned => resolved
2019-03-21 00:30 wp Resolution reopened => fixed
2019-03-21 00:30 wp Target Version => 2.2
2019-04-06 03:30 Marcin Wiazowski File Added: patch2.diff
2019-04-06 03:31 Marcin Wiazowski Note Added: 0115260
2019-04-06 03:31 Marcin Wiazowski Status resolved => assigned
2019-04-06 03:31 Marcin Wiazowski Resolution fixed => reopened
2019-04-06 10:25 wp Note Added: 0115261
2019-04-06 10:25 wp Fixed in Revision 60740 => 60740, 60856
2019-04-06 10:25 wp Status assigned => resolved
2019-04-06 10:25 wp Resolution reopened => fixed
2019-04-08 19:06 Marcin Wiazowski Note Added: 0115331
2019-04-08 19:06 Marcin Wiazowski Status resolved => assigned
2019-04-08 19:06 Marcin Wiazowski Resolution fixed => reopened
2019-04-09 11:16 wp Note Added: 0115340
2019-04-09 23:02 Marcin Wiazowski File Added: revert.diff
2019-04-09 23:02 Marcin Wiazowski File Added: patch3.diff
2019-04-09 23:03 Marcin Wiazowski Note Added: 0115364
2019-04-10 09:53 wp Note Added: 0115380
2019-04-10 09:53 wp Fixed in Revision 60740, 60856 => 60856
2019-04-10 09:53 wp Status assigned => resolved
2019-04-10 09:53 wp Resolution reopened => fixed
2019-04-10 09:53 wp Status resolved => assigned
2019-04-10 09:53 wp Resolution fixed => reopened
2019-04-10 10:02 wp Note Added: 0115382
2019-04-10 13:57 Marcin Wiazowski Note Added: 0115388
2019-04-10 14:27 wp Fixed in Revision 60856 => 60856, 60918
2019-04-10 14:27 wp Status assigned => resolved
2019-04-10 14:27 wp Resolution reopened => fixed
2019-04-10 14:27 wp Status resolved => assigned
2019-04-10 14:27 wp Resolution fixed => reopened
2019-04-10 14:29 wp Note Added: 0115389
2019-04-10 14:29 wp Status assigned => resolved
2019-04-10 14:29 wp Resolution reopened => fixed
2019-04-10 16:17 Marcin Wiazowski Note Added: 0115391
2019-04-10 16:17 Marcin Wiazowski Status resolved => closed