ChartDataSet calcYValueSum() bug

[yoncise]

notifyDataSetChanged() will call calcYValueSum()

private func calcYValueSum()
{
    _yValueSum = 0

    for var i = 0; i < _yVals.count; i++
    {
        _yValueSum += fabs(_yVals[i].value)
    }
}

Sum calculated from addEntry() and calcYValueSum() is not same, if value is negative. I think fabs() is not needed.

[liuxuan30]

Good catch! It smells like a bug, but I cannot find a proper fix since I found that it is (only?) used by pie chart, and speaking of angles in pie chart, negative values is supposed to be positive ones?

@danielgindi what do you think? Any charts except pie chart is also using yValueSum? Should it be fabs all the time, or we need to remove fabs?

There are other funcs such as removeFirst(), removeLast(), removeEntry(), addEntryOrdered() , removeDataSetByIndex(), addDataSet(), are not using fabs

[yoncise]

LineChart also use calcYValueSum. Actually, I find this issue because my line chart which contains negative value has wrong average value.

[liuxuan30]

which func in LineChart use yValueSum? Calculating yValueSum but not using it is harmless. I just found pie chart is using yValueSum

[yoncise]

As my title said, it's ChartDataSet's calcYValueSum() used fabs() not ChartView's. ChartDataSet is the base class for all DataSet, so I think use fabs() in calcYValueSum() is not reasonable.

[liuxuan30]

You can search who is using _yValueSum, not just calcYValueSum(). Not every chart will use it, even it is calculated. Otherwise people will find this bug earlier I suppose since abs() or not is quite different.

[yoncise]

I find this "bug" because I called notifiedDataSet on ChartDataSet which is not stated on doc (maybe) instead of ChartView. So, maybe this is reason why this bug is not found before. Most people will only call notifiedDataSet on ChartView as doc said.

[danielgindi]

Pie values should actually just not be negative. It's not valid. There's no good way to represent negative values in the pie chart (except maybe for going back in the other direction with a different width or color).

Anyway I'm looking into the yValueSum thing

[danielgindi]

@PhilJay I can't find any reason for it being absolutized... Probably something historic but git's blame can only go a certain distance.

There's nothing using yValueSum except for the Pie chart. I think we should remove the abs right?

[PhilJay]

Yes, the abs can be removed!

[liuxuan30]

@danielgindi how about pie chart? We don't abs the negative values any more?
Just tried, no colorful pies :)

[danielgindi]

No we shouldn't. Pie charts should not contain negative values... And if they do - they definitely should not increase the total but render in a different way.
Have you encountered a pie chart with negative values?

[liuxuan30]

I only encountered with test data or some mistaken input. I'm ok to let user abs it rather than us.

[danielgindi]

I think that it also has another advantage - the user will immediately see that there's something wrong with his data, instead of seeing "correct" data and not being able to understand why it doesn't match another report... :-)