Fix #divmod #3426

denisdefreyne · 2016-10-16T10:48:33Z

Two changes:

The first element of the tuple should be an integer (quotient rounded down), rather than a float containing the quotient itself.
Documentation for negative integer divmod is now correct, and has additional tests for the examples.

Before:

1.4.divmod(0.3)[0]
# => 4.6666…

After:

1.4.divmod(0.3)[0]
# => 4

The first element of the tuple should be an integer (quotient rounded down), rather than a float containing the quotient itself.

yxhuvud · 2016-10-16T11:09:41Z

spec/std/float_spec.cr

+
+    assert { 1.4.divmod(0.3)[0].should eq(4) }
+    assert { 1.4.divmod(0.3)[1].should be_close(0.2, 0.00001) }
+  end


Are the results this code will get for negative numbers considered? As it stands, it doesn't look like that, considering that the commit message claims the numbers are rounded down (which is not true - they are rounded towards 0) and that there are no tests for the negative case.

As a comparison, Numeric#divmod in Ruby is defined to round towards -infinity.

Ahh, good point—I did not have that in mind. I’ll add some more test cases and fix the implementation.

yxhuvud · 2016-10-16T11:13:28Z

src/number.cr

@@ -158,7 +158,7 @@ struct Number
  # 11.divmod(-3) # => {-3, 2}


Interestingly enough, in Ruby this gives:

11.divmod -3
=> [-4, -1]

I'm pretty sure Python's behavior completely matches Ruby's behavior. They should be closely followed.

denisdefreyne · 2016-10-16T12:09:13Z

I’ve updated the divmod tests, documentation and implementation to (hopefully) account for all cases.

The behavior of divmod is unfortunately up for discussion. For example, 11.divmod(-3) could be implemented as returning {-4, -1} (rounding towards -infinity) or {-3, 2} (rounding towards 0), and both are valid. (Ruby does the former.)

oprypin · 2016-10-16T12:33:23Z

Before:

11.divmod(-3)
# => {-3, 2}

That is not true, only the example in the documentation was wrong. The PR doesn't change this behavior.

The first element of the tuple should be an integer

No, it shouldn't be an actual Int, because some Floats don't fit into an Int. But the PR uses floor which does actually return a Float, so all is good.

I completely agree with this pull request now, it doesn't change any behavior that was correct, and only fixes the output for Floats. The behavior matches that of Ruby and Python.

denisdefreyne · 2016-10-16T20:02:06Z

Correct, my bad! I’ll update the description.

asterite · 2016-10-17T17:57:06Z

@ddfreyne Thank you!!

chocolateboy · 2016-10-17T18:32:22Z

Looks like the second example in the documentation was missed:

python

divmod(11, -3) # => (-4, -1)

ruby

11.divmod(-3) # => [-4, -1]

crystal

11.divmod(-3) # => {-4, -1}

documentation

# 11.divmod(-3) # => {-4, 1}

Fix #divmod for Float

e9ccff5

The first element of the tuple should be an integer (quotient rounded down), rather than a float containing the quotient itself.

yxhuvud reviewed Oct 16, 2016

View reviewed changes

denisdefreyne added 3 commits October 16, 2016 13:56

fixup! Fix #divmod for Float

34dd978

fixup! Fix #divmod for Float

6280b55

fixup! Fix #divmod for Float

c348f04

denisdefreyne changed the title ~~Fix #divmod for Float~~ Fix #divmod Oct 16, 2016

oprypin approved these changes Oct 16, 2016

View reviewed changes

asterite added kind:bug topic:stdlib labels Oct 17, 2016

asterite merged commit ae551e9 into crystal-lang:master Oct 17, 2016

denisdefreyne deleted the denis/bug/float-divmod-int branch October 17, 2016 18:25

chocolateboy mentioned this pull request Oct 17, 2016

Fix doc typo for Number#divmod #3432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #divmod #3426

Fix #divmod #3426

denisdefreyne commented Oct 16, 2016 •

edited

Loading

yxhuvud Oct 16, 2016

denisdefreyne Oct 16, 2016

yxhuvud Oct 16, 2016

oprypin Oct 16, 2016

denisdefreyne commented Oct 16, 2016 •

edited

Loading

oprypin commented Oct 16, 2016

denisdefreyne commented Oct 16, 2016

asterite commented Oct 17, 2016

chocolateboy commented Oct 17, 2016 •

edited

Loading

		@@ -158,7 +158,7 @@ struct Number
		# 11.divmod(-3) # => {-3, 2}

Fix #divmod #3426

Fix #divmod #3426

Conversation

denisdefreyne commented Oct 16, 2016 • edited Loading

yxhuvud Oct 16, 2016

Choose a reason for hiding this comment

denisdefreyne Oct 16, 2016

Choose a reason for hiding this comment

yxhuvud Oct 16, 2016

Choose a reason for hiding this comment

oprypin Oct 16, 2016

Choose a reason for hiding this comment

denisdefreyne commented Oct 16, 2016 • edited Loading

oprypin commented Oct 16, 2016

denisdefreyne commented Oct 16, 2016

asterite commented Oct 17, 2016

chocolateboy commented Oct 17, 2016 • edited Loading

python

ruby

crystal

documentation

denisdefreyne commented Oct 16, 2016 •

edited

Loading

denisdefreyne commented Oct 16, 2016 •

edited

Loading

chocolateboy commented Oct 17, 2016 •

edited

Loading