Make FD÷FD return an FD.

[NHDaly]

Before this, FD{T,f} ÷ FD{T,f} returned T, now it returns FD{T,f}.

Same for div, fld, and fld.

---

Here's an example of the old behavior. Addition and division for two FixedDecimals returns a FixedDecimal, but integer division (div) was returning the raw storage type T instead of wrapping it in a FixedDecimal.

julia> FixedDecimal{Int32, 2}(2)+FixedDecimal{Int32, 2}(1)
>> FixedDecimal{Int32,2}(3.00)

julia> FixedDecimal{Int32,2}(4.00) / FixedDecimal{Int32,2}(2.00)
>> FixedDecimal{Int32,2}(2.00)

julia> FixedDecimal{Int32, 2}(2)÷FixedDecimal{Int32, 2}(1)
>> 2

[coveralls]

Coverage increased (+0.02%) to 96.471% when pulling 1819dd6 on NHDaly:div_return_FD into 07d24d9 on JuliaMath:master.

[codecov-io]

Codecov Report

Merging #40 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   98.82%   98.83%   +<.01%     
==========================================
  Files           1        1              
  Lines         170      171       +1     
==========================================
+ Hits          168      169       +1     
  Misses          2        2

Impacted Files	Coverage Δ
src/FixedPointDecimals.jl	98.83% <100%> (ø)	⬆️

---

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 1768c58...dceec29. Read the comment docs.

[NHDaly]

Also, I noticed that there weren't any unit tests for div, so I added a few simple ones.

[TotalVerb]

One advantage of returning T is that there's no danger of overflow, whereas when returning the FD there is. Is there a reason why the FD is desirable here?

[omus]

Since / returns a FD I do think it makes sense to do the same for div. We probably do need to have some of the widening logic from / here as well though.

[NHDaly]

One advantage of returning T is that there's no danger of overflow, whereas when returning the FD there is. Is there a reason why the FD is desirable here?

Yeah, this is a good point, @TotalVerb. By returning a T right now, we avoid overflows in places where it would happen otherwise, e.g.:

julia> FixedPointDecimals.FixedDecimal{Int16, 4}(1) ÷ FixedPointDecimals.FixedDecimal{Int16, 4}(0.0005)
>> 2000

julia> FixedPointDecimals.FixedDecimal{Int16, 4}(2000)
ERROR: InexactError: trunc(Int16, 20000000)

That said, I agree with omus that since all the other operations (such as /) return an FD, I think it makes sense here as well. For example, if + returned an integer it, too, could avoid overflows but that wouldn't really make sense. That said, this one is a bit different than + since it has to do a bit of work to convert the result back to a FD when the actual operation itself causes you to produce the integer representation... But I dunno, it just seems weird for users to need to remember to convert back to an FD every time they integer divide them. And if the user really wants to get the int representation back, they can always just divide their internal values: x.i ÷ y.i?

---

@omus commented 3 days ago
We probably do need to have some of the widening logic from / here as well though.

Actually, I don't think we do, although it would probably be worth adding a comment explaining why we don't.
When you divide the two FixedDecimals, is you automatically eliminate all of the fractional part of the answer (FD{Int,2}(3) ÷ FD{Int,2}(2) --> 300÷200 == 1).

/ needs to widen the inputs and multiply by 10^f ahead of time, specifically to avoid eliminating the fractional part of the answer, since we don't want to lose precision. But in the case of truncating integer division (÷), "eliminating the fractional part of the answer" is exactly what we want! So it should be enough as is, I think.

[NHDaly]

Maybe the comment could be something like:

    # Note that div(x.i, y.i) eliminates the scaling coefficient, so we need to reconstruct the FD.

[NHDaly]

Added a comment here to explain why we don't need the widening logic. Does this look okay?:

FixedPointDecimals.jl/src/FixedPointDecimals.jl

Lines 307 to 311 in becc3df

	for divfn in [:div, :fld, :fld1]
	# div(x.i, y.i) eliminates the scaling coefficient, so we call the FD constructor.
	# We don't need any widening logic, since we won't be multiplying by the coefficient.
	@eval $divfn(x::T, y::T) where {T <: FD} = T($divfn(x.i, y.i))
	end

[omus]

Tests need a little work otherwise I'm happy to merge.

[NHDaly]

Please take another look; should be good to merge now, i think! :)