kaitai_struct icon indicating copy to clipboard operation
kaitai_struct copied to clipboard

Unclear expected values for integer division

Open Mingun opened this issue 3 months ago • 4 comments

Currently Java test tests\formats\expr_int_div.ksy is failed with the following message:

java.lang.AssertionError: expected [-757] but found [-756]
	at org.testng.Assert.fail(Assert.java:94)
	at org.testng.Assert.failNotEquals(Assert.java:513)
	at org.testng.Assert.assertEqualsImpl(Assert.java:135)
	at org.testng.Assert.assertEquals(Assert.java:116)
	at org.testng.Assert.assertEquals(Assert.java:389)
	at org.testng.Assert.assertEquals(Assert.java:399)
	at io.kaitai.struct.spec.CommonSpec.assertIntEquals(CommonSpec.java:36)
	at io.kaitai.struct.spec.TestExprIntDiv.testExprIntDiv(TestExprIntDiv.java:17)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:607)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:86)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:643)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:820)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1128)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at org.testng.TestRunner.privateRun(TestRunner.java:782)
	at org.testng.TestRunner.run(TestRunner.java:632)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:366)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:361)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:319)
	at org.testng.SuiteRunner.run(SuiteRunner.java:268)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1244)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1169)
	at org.testng.TestNG.run(TestNG.java:1064)
	at org.testng.TestNG.privateMain(TestNG.java:1385)
	at org.testng.TestNG.main(TestNG.java:1354)

I suggest instead of testing strange numbers just test division of everything in range [-10; 10] by 3 and -3, the same for mod.

Mingun avatar Mar 18 '24 19:03 Mingun

To be honest, I don't see that big of a difference — it merely checks out two use cases (positive and negative) times two sources (constant or value coming from a file).

But, well, [-10; 10] will work the same too. Feel free to implement ;)

GreyCat avatar Mar 19 '24 11:03 GreyCat

The question what is expected results for -10...10 / 3 and -10...10 / -3? Different languages seems to have different opinions and this opinion is not covered by tests. The result can depend on rounding and it is better to explicitly differentiate, we expect to get floor(a / b), ceil(a / b), or round(a / b) (assuming here that it is float division) and with what sign.

Mingun avatar Mar 19 '24 17:03 Mingun

The question what is expected results for -10...10 / 3 and -10...10 / -3?

Sometimes I can't decode what you mean by your sentences - is that your question? Did you not see the comment at the top of the KSY spec expr_int_div.ksy that you were pointing to?

expr_int_div.ksy:1-3

# Tests division operation, both positive and negative
# See https://github.com/kaitai-io/kaitai_struct/issues/746
#  => the KS division operation `a / b` should do `floor(a / b)`

I think this states the expected behavior clear enough and it even provides a link to https://github.com/kaitai-io/kaitai_struct/issues/746 with detailed explanation.

The result can depend on rounding

Hypothetically, it probably could if the corresponding translator method of some target in KSC was implemented by someone who doesn't know the correct semantics, but as far as I know, it doesn't in any target language at the moment (i.e. (4 / 3) == (5 / 3) is true in all languages, even though mathematically 4 / 3 = 1.33 and 5 / 3 = 1.67, so if the integer division result depended on rounding, they wouldn't be equal - see also the table in https://github.com/kaitai-io/kaitai_struct/issues/746#issue-613681200 with real results from all target languages).

just test division of everything in range [-10; 10] by 3 and -3

This sounds like the table @dgelessus made (the columns to look at would be floor(i / 3) and "i % 3 behavior 2"), see the expandable section at the bottom of https://github.com/kaitai-io/kaitai_struct/issues/746#issuecomment-624965399 that says:

▸ Table to compare division and modulo behaviors


As @GreyCat said, feel free to improve the test as you see fit, at least we'll have something concrete to discuss.

generalmimon avatar Mar 19 '24 18:03 generalmimon

Did you not see the comment at the top of the KSY spec expr_int_div.ksy that you were pointing to?

Yes, strangely, but it is true. I really didn't notice it.

The investigation in #746, however, somehow ignored the issue with a negative divisor.

Also for me it is strange that as the test use of strange numbers which result of division it is heavy to calculate manually and that only couple of values from all (incomplete) described table are checked was chosen. I'll prepare PR with exhaustive tests soon. I only hope that it will not stuck as other my (and not only my) PRs here.

Mingun avatar Mar 20 '24 05:03 Mingun