StackOverflowError when calling ChartEntryModelProducer::setEntries() about vico HOT 8 CLOSED

Vico 1.14.0 Beta 1 also includes the aforementioned improvement.

from vico.

Hello! Thanks for the report. The chart data is relevant here. Could you please share a set of x and y values for which the crash occurs? You could, for example, update your code to send a log message containing the x and y values before setEntries is invoked.

from vico.

I struggle to reproduce it right now. I'll post the entries once, I get the StackOverflowError again.
Instead I get this error, even though there are no duplicate values:

FATAL EXCEPTION: DefaultDispatcher-worker-1
                                                                                                    Process: redacted, PID: 20412
                                                                                                    java.lang.IllegalArgumentException: The precision of the x values is too large. The maximum is two decimal places.
                                                                                                    	at com.patrykandpatryk.vico.core.entry.ChartEntryExtensionsKt.calculateXGcd(ChartEntryExtensions.kt:94)
                                                                                                    	at com.patrykandpatryk.vico.core.entry.ChartEntryModelProducer.getInternalModel(ChartEntryModelProducer.kt:151)
                                                                                                    	at com.patrykandpatryk.vico.core.entry.ChartEntryModelProducer.getInternalModel$default(ChartEntryModelProducer.kt:133)
                                                                                                    	at com.patrykandpatryk.vico.core.entry.ChartEntryModelProducer.getModel(ChartEntryModelProducer.kt:158)
                                                                                                    	at com.patrykandpatryk.vico.core.entry.ChartEntryModelProducer$UpdateReceiver.handleUpdate(ChartEntryModelProducer.kt:213)
                                                                                                    	at com.patrykandpatryk.vico.core.entry.ChartEntryModelProducer.registerForUpdates(ChartEntryModelProducer.kt:190)
                                                                                                    	at com.patrykandpatryk.vico.compose.chart.entry.ChartEntryModelExtensionsKt$collectAsState$1$1.invokeSuspend(ChartEntryModelExtensions.kt:116)
                                                                                                    	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                                                                                                    	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:793)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:697)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:684)
                                                                                                    	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [androidx.compose.ui.platform.MotionDurationScaleImpl@e1a8da, androidx.compose.runtime.BroadcastFrameClock@423810b, StandaloneCoroutine{Cancelling}@df17be8, Dispatchers.Default]

Here are the values I put in setEntries():
precision-too-large-exception.txt

I suspect a concurrency problem, but don't know how to debug it further.

from vico.

I wrote a test-case with the FloatEntries from my last comment. (I copied the internal implementation from Vico files, so I could test them):
ChartEntryExtensionsTest.kt.txt

There seems to be a bug in calculateXGcd(): The last FloatEntry causes the exception to incorrectly being thrown. If you comment it out, no exception is thrown. The x-value of that entry is unique up to the first three digits (864xx.x) which should be enough?

Edit after some tinkering around with the Vico-code:
I think the problem is in this function (I added a println):

fun Iterable<Iterable<ChartEntry>>.calculateXGcd() =
        flatten().zipWithNext { firstEntry, secondEntry -> abs(secondEntry.x - firstEntry.x) }
            .fold<Float, Float?>(null) { gcd, delta ->
                println("GCD between $gcd and $delta: ${gcd?.gcdWith(delta) ?: delta}")
                gcd?.gcdWith(delta) ?: delta }
            ?.also { require(it != 0f) { "The precision of the x values is too large. The maximum is two decimal places." } }
            ?: 1f

The issue is the abs(secondEntry.x - firstEntry.x) code. Floats are not precise enough to not introduce errors when interacting like this. e.g. 74176.98f - 46336.89f == 27840.086f but should be 27840.09.
Here's a test, which fails:

// this fails!
@Test
fun `float subtract test`() = runTest {
    assertEquals("should be 27840.09", 27840.09f, 74176.98f - 46336.89f)
}

By introducing a third digit right of the decimal point, the GCD between 0.01 and 27840.086 is now 0.001 and used as 0f in the next iteration, when it should still be 0.01f.
So the solution might be to round after the abs()-call like this abs(secondEntry.x - firstEntry.x).round(2)?

Also, I think the output of the println() should print decreasing GCDs(?), but due to the imprecision it reaches 0f sometimes, and is replaced in the next iteration with the new delta, which is (usually) higher again. Only if the last delta produces 0f, the Exception is correctly thrown.

Edit 2: Rounding the abs() call is not correct. It would let the following test fail, which currently succeeds:

@Test
fun `test xcheck fails`() = runTest {
    assertThrows<IllegalArgumentException> {
        val series: List<List<ChartEntry>> = listOf(
            listOf(
                FloatEntry(x = 0.0f, y = 1.460994f),
                FloatEntry(x = 0.001f, y = 11.586849f),
                FloatEntry(x = 0.01f, y = 11.362893f),
            )
        )
        series.calculateXGcd()
    }
}

I think the safest thing to do is to only use whole numbers (Ints or Longs) for the x-values. Then it should be possible to find the correct GCD. Already the first cast to Float in entryOf() introduces precision errors.
Alternatively, ditch the GCD search and trust the Vico-user to give Vico valid Entries?

from vico.

Thanks for looking into this, @littleant! It’s not necessarily a deal-breaker that the first proposed solution causes the last test to fail. The GCD algorithm is in place not to detect excessive precision, but rather to determine the default x step. The zero check is present because an x step of zero would cause unexpected behavior. It happens to be the case that calculateXGcd produces such an x step for overly precise x values. Such x values can cause other issues too, and there already are instances in which they go undetected—perhaps a dedicated precision check should be added.

The round(2) solution could be added if it fixed the delta calculation. Unfortunately, from what I can tell, it doesn’t necessarily produce the desired result. While it works for 74176.98f - 46336.89f, it fails for 174176.98f - 146336.83f, where the result would have to be rounded down to be correct.

It does look like something has to be changed here. Since, as described above, calculateXGcd determines the default x step, removing the GCD search would constitute a regression. More specifically, the default scaling of charts would worsen. This isn’t optimal, but it does seem like less of an inconvenience than switching to Ints or Longs, which would make it impossible to use fractional x values, forcing Vico users to do more data processing. If calculateXGcd is removed, it might be possible to add it back once we switch from Floats to Doubles, which we plan on doing. I’ll discuss the matter with @Gowsky.

In the meantime, I suggest that you multiply your x values by 10 (for one decimal place) or, if that doesn’t help, by 100 (for whole numbers). You can divide them during formatting to have the right values displayed along x-axes.

from vico.

A dedicated precision check would be a solution I think. It would assert that there are no values with a precision lower than 0.01. Afterwards you can define the minimum GCD to be 0.01, regardless of what you detect in calculateXGcd().

I agree, round(2) is not the solution here.

I think switching to Double would not solve the issue. It would just allow larger values until the same issue surfaces again. (You are just switching from 32bit to 64bit.)
Unless you keep the external API to only use Floats and immediately convert them to Doubles for internal use (e.g. in entryOf(). This may work, I don't know.

If you don't want to switch to Int or Long in the external API, you could convert to Longs in the internal API, since you already restrict the precision. e.g. with this conversion logic:

• If x-value is of type Float, cast it to Double, multiply it by 100 and cast it to Longs for internal use.
• If x-value is of type Double, multiply by 100 and cast it to Longs. (This could be a problem with higher values)
• If x-value is of type Int, cast to Long and multiply by 100.
• If x-value is of type Long, multiply by 100. (This could also lead to problems, if values near Long.MAX_VALUE are being used.)

I have added this logic in the hopes that it prevents exceptions. (I'm not sure if .toInt().toFloat() actually does anything except hiding fractional digits?):

.setEntries(entries.map { entry ->
    FloatEntry(x = (entry.x * 100f).toInt().toFloat(), y = entry.y)
})

from vico.

Thanks for the input, @littleant! Regarding switching to Floats to Doubles, this problem would certainly reappear for some precision level, but what I had in mind is that Doubles might be precise enough to give us a reasonably high precision level that consistently works. We’d still need a precision check. As can be seen here, with Floats, even two decimal places can be problematic, and a limit of one decimal place would be a bit too restrictive. About chaining toInt and toFloat, this doesn’t do anything beyond removing all decimals. A slightly more concise option is floor(entry.x * 100f).

Vico 2.0.0 Alpha 6 introduces an improvement that is helpful here: when you set a custom x step, no GCD calculations are run. Thus, when the GCD algorithm fails due to precision issues, rather than transforming your data, you can simply explicitly specify the x step. For your data, getXStep = { 0.01f } could be added, and everything should work as expected. Of course, it would be optimal for the GCD algorithm not to fail in the first place. We’ll continue to look into this.

from vico.

As I understand, the StackOverflowError problem hasn’t reappeared, so I’ll be closing this issue for now. If the crash occurs again, please feel free to reopen the issue. We’ll be sharing updates concerning the switch from Float to Double under #623. Cheers!

from vico.