Fixed, If the last value is the max or min, the range will be wrong

[aelam]

The last entry is ignored.
If the last value is the max or min, the range will be wrong

[aelam]

Another problem.
How about just one entry?
The max/min are the same, if the autoScale is enabled, the charts doesn't know the range because it is 0, right?

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@1ed1561). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #2229   +/-   ##
=========================================
  Coverage          ?   18.44%           
=========================================
  Files             ?      124           
  Lines             ?    14619           
  Branches          ?        0           
=========================================
  Hits              ?     2696           
  Misses            ?    11923           
  Partials          ?        0

Impacted Files	Coverage Δ
...s/Data/Implementations/Standard/ChartDataSet.swift	33.55% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed1561...30518c6. Read the comment docs.

[liuxuan30]

Is there any issue to track this PR? we have no context for this change.

[aelam]

I didn't notice other issues about this.
Let me tell you my case

1. XAxis can scale , YAxis can autoScale
2. ChartView just has 2 CandleStick entries(other type Data is the same, I think), and the second one
   is the larger one

Go through this case
My visible XValues is 1 ,2, my XAxis is from 0.5 to 2.5
When goes into calcMinMaxY
indexFrom = 0, indexTo = 1
and the indexFrom..<indexTo is an open range
that means it just calculate one value
if it were indexFrom...indexTo, it would work fine

Case 2: Just one entry
There is only one entry. then indexTo = 0, indexFrom = 0
when goes into if indexTo <= indexFrom { return } DONE
it will never run into the for-loop to calculate.
Then I can't get the correct Y range anyway

Case 3: Based on the case 2, Just one entry
and the YMax and YMin is the same!,
What can I do?

So the correct code to fix the first two problems I think it may be like this

open override func calcMinMaxY(fromX: Double, toX: Double)
         let indexFrom = entryIndex(x: fromX, closestToY: Double.nan, rounding: .down)
         let indexTo = entryIndex(x: toX, closestToY: Double.nan, rounding: .up)
          
          if indexTo < indexFrom { return }
          
       for i in indexFrom...indexTo {
            ...
       }

[aelam]

I just checked the android version .
We are different here

@Override
    public void calcMinMaxY(float fromX, float toX) {

        if (mValues == null || mValues.isEmpty())
            return;

        mYMax = -Float.MAX_VALUE;
        mYMin = Float.MAX_VALUE;

        int indexFrom = getEntryIndex(fromX, Float.NaN, Rounding.DOWN);
        int indexTo = getEntryIndex(toX, Float.NaN, Rounding.UP);

        for (int i = indexFrom; i <= indexTo; i++) {

            // only recalculate y
            calcMinMaxY(mValues.get(i));
        }
    }

[aelam]

@xuan @danielgindi could you please verify this

[liuxuan30]

@danielgindi should be still busy.. Haven't seen him recently. As this is a foundermental change, we need to have @danielgindi to review.

[aelam]

😒 I think it's easy to reproduce this bug, you can verify it. And it's very reasonable, also android code can support it. 发自我的 iPhone

…

在 2017年4月1日，上午10:34，Xuan ***@***.***> 写道： @danielgindi should be still busy.. Haven't seen him recently. As this is a foundermental change, we need to have @danielgindi to review. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[liuxuan30]

@danielgindi please take a look at this PR

[liuxuan30]

@aelam I am taking this PR right now, some thoughts:

   let indexTo = entryIndex(x: toX, closestToY: Double.nan, rounding: .up)

This is rounding .up, which means indexTo may be larger by 1, which would cause oob issue, right?

I took a look at open override func entryIndex(x xValue: Double, closestToY yValue: Double, rounding: ChartDataSetRounding) -> Int, actually quite complicated than I expect, though android use <= at the moment.

We have to check the boundary again.
In order to make sure ... works without crash, first we need to verify

       if closest != -1
        {
            let closestXValue = _values[closest].x
            
            if rounding == .up
            {
                // If rounding up, and found x-value is lower than specified x, and we can go upper...
                if closestXValue < xValue && closest < _values.count - 1
                {
                    closest += 1
                }
            }

that closest when it's the last entry, and it won't get +1 that will lead to crash.

Then, in another condition:

            // Search by closest to y-value
            if !yValue.isNaN
            {
                while closest > 0 && _values[closest - 1].x == closestXValue
                {
                    closest -= 1
                }
                
                var closestYValue = _values[closest].y
                var closestYIndex = closest
                
                while true
                {
                    closest += 1
                    if closest >= _values.count { break }
                    
                    let value = _values[closest]
                    
                    if value.x != closestXValue { break }
                    if abs(value.y - yValue) < abs(closestYValue - yValue)
                    {
                        closestYValue = yValue
                        closestYIndex = closest
                    }
                }
                
                closest = closestYIndex
            }
        }

Here

closest += 1
if closest >= _values.count { break }

I guess that's why it uses ..< in the first place.

However, in

let indexTo = entryIndex(x: toX, closestToY: Double.nan, rounding: .up)

It passes closestToY: Double.nan, so thank god the second if !yValue.isNaN is not hit. Now I think it's safe to use ... now.

Share your thoughts if you can come up any other potential risks.

I think this is positive to merge.

[liuxuan30]

should be safe to merge then with the analysis above and CI tests.
@danielgindi @petester42 do you want to review this? As this may impact a lot. I want to make sure we don't miss any cases

[emmaviolet]

@danielgindi @petester42 are there any plans to release a new version with the latest changes? This is a useful bugfix 🙏

[liuxuan30]

It's already merged, so you can easily build one, or take the change and make a patch