Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unclear expected values for integer division #1096

Open
Mingun opened this issue Mar 18, 2024 · 4 comments
Open

Unclear expected values for integer division #1096

Mingun opened this issue Mar 18, 2024 · 4 comments

Comments

@Mingun
Copy link

Mingun commented Mar 18, 2024

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.

@GreyCat
Copy link
Member

GreyCat commented Mar 19, 2024

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 ;)

@Mingun
Copy link
Author

Mingun commented Mar 19, 2024

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.

@generalmimon
Copy link
Member

generalmimon commented Mar 19, 2024

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 #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 #746 (comment) 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 #746 (comment) 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.

@Mingun
Copy link
Author

Mingun commented Mar 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants