Money.split returning negative remainders

[jdewar]

Perhaps this is working as intended, but I was surprised to get a negative number.

iex> Money.split(Money.new( :USD, 200), 3)
{Money.new(:USD, "66.67"), Money.new(:USD, "-0.01")}

One solution would be rounded_money = Money.round(money, rounding_mode: :down) at the top of def split, I think

[kipcole9]

Definitely a bug, thanks much for reporting it. I’m very surprised actually so I’ll dig into this asap - but I’m very internet access challenged at the moment so I can’t commit exactly when I can get it done.

[jdewar]

I came across this while trying to write my own 'split' function using Money.split:

• I have a total on one line item with > 1 quantity,
• apply a discount,
• want to calculate the new value of each unit in that line item
  I want to return something like [66.66, 66.67, 66.67], actually [{66.66, 1}, {66.67, 2}] to avoid long lists when splitting by large numbers).

Is there a function in Money to deal with smallest units that I've missed? This was how I ended up going about it

def split(total, parts) do
  {currency, smallest_units, _digits, _remainder}  = Money.to_integer_exp(total)
  even_split = Kernel.div(smallest_units, parts)
  remainder = rem(smallest_units, parts)
  [
    {Money.from_integer(even_split, currency), parts - remainder},
    {Money.from_integer(even_split + 1, currency), remainder}
  ]
end

[{Money.new(:USD, "66.66"), 1}, {Money.new(:USD, "66.67"), 2}]

[jdewar]

Sorry to spam. Was talking with a friend about this and the mental wheels are still turning. My first suggestion is wrong; it should've been the second round, the one that is assigned to div. Is the first round necessary to do a div?

Also I see that my 'correct' calculation above is wrong! I want 66.66, 66.67 and 66.67. Going to edit the code in place, so there's not incorrect code masquerading as an answer

[kipcole9]

Sorry for the delay, I'm working on this now.

I would like to keep the default rounding where possible. So I'm thinking to apply that first, and only if the remainder is negative recalculate using :down. This should result in the largest possible factor with the smallest possible remainder. An assertion that needs testing of course, and I'll do some property tests as well.

The reason that the first rounding happens is that the amount can be arbitrary precision. And therefore it needs to be rounded to the precision of the currency before splitting. This is a design choice of course - the assumption being that Money.split/2 is going to be used to allocate real money and therefore triggers rounding - also so we can calculate the remainder.

Comments welcome.

[kipcole9]

I've push a commit that implements the strategy above. Would you consider giving that a test drive before I publish to hex? Comments/suggestions welcome.

[kipcole9]

I came across this while trying to write my own 'split' function using Money.split

I'm having trouble working out what you're trying to do here. I think you mean you want to do something the minor unit of a currency (like cents). Not enough coffee is probably the problem. Can you explain a little more what you're trying to do?

[jdewar]

Haven't forgotten about this, thanks for the commit! Will let you know when time is available.

[jdewar]

Can you explain a little more what you're trying to do?

I think the quickest way to put it is that I want to distribute the remainder as evenly as possible amongst the requested # of parts. In the above example I want split to return 3 money structs worth [66.66, 66.67, 66.67].

Is there a function in Money to deal with smallest units that I've missed?

Earlier I asked the above because with the current split return value of {66.66, 0.02}, I didn't see a way to distribute that 2 cents evenly, or to know that there were '2' of them. My solution uses to_integer_exp bc I can distribute the remainder once I have things in a 'count' of the smallest currency unit.

In slightly longer form, if I have N units at P price and I apply D discount on the total, I want to know how much each of the N units is worth now, for handling per-unit values like taxes, duties, return values (how much do I refund when they return 1 item), etc.

[coladarci]

For what it's worth, we also deeply care about splitting money and wrote a splitter on top of Money to do it, it looks like this:

  @doc """
  Splits a given money amount into evenly divided portions, and allows the first n of those to be added together.

  ## Options

    * `:currency_digits` - Any values allowed for `Money.round/2` are valid here. `:cash`, `:iso`, `:accounting`.  Cash
      rounding will respect the minimum cash unit of the currency, e.g. CAD is 0.05.

  ## Example

  I have a $100 bill. I'm a party of 5. I want to split it evenly, but pay for me and my wife.
  Returns [$40, $20, $20, $20]. First is the "pay now". The rest of the list are the rest of the payments.
  """
  def split_money(%Money{} = money, num_splits, num_to_charge_now \\ 1, opts \\ [])
      when num_splits >= 1 and num_to_charge_now >= 1 and num_to_charge_now <= num_splits do
    {_, max_splits, _, _} = Money.to_integer_exp(money)
    max_splits = max(abs(max_splits), 1)

    if num_splits > max_splits, do: raise(ArgumentError, "Can't split #{inspect(money)} into #{num_splits} pieces.")

    splits =
      money
      |> do_split_money(num_splits, [], opts)
      # put larger values at the end of the list
      |> Enum.sort(fn m1, m2 -> Money.compare!(m1, m2) in [:lt, :eq] end)

    if num_to_charge_now > 1 do
      {charge_now, charge_later} = Enum.split(splits, num_to_charge_now)
      [first | rest] = charge_now
      charge_now_amount = Enum.reduce(rest, first, &Money.add!/2)

      [charge_now_amount | charge_later]
    else
      splits
    end
  end

  defp do_split_money(money_left, 1, splits, _), do: Enum.reverse([money_left | splits])

  defp do_split_money(money_left, splits_left, splits, opts) do
    this_split = money_left |> Money.div!(splits_left) |> Money.round(opts)
    do_split_money(Money.sub!(money_left, this_split), splits_left - 1, [this_split | splits], opts)
  end

[kipcole9]

Thank you both, this is very helpful. The intent of split/2 is as you both describe - evenly split into n parts with a reminder that cannot - in the content of the given currency - be split equally amongst the n parts.

@coladarci thank you sharing you implementation, it helps me understand where my implementation is not meeting the expectation.

I’ll jump into this again when I’m on a computer in the morning.

Partially the issue/opportunity is to decide when the split occurs - before the initial rounding to the currency’s precision or after.

Do either of you have a strong view on this?

[Wigny]

I want to distribute the remainder as evenly as possible amongst the requested # of parts.

Recursion could be handy here, right?

def split(_money, 0) do
  []
end

def split(money, parts) do
  {dividend, _remainder} = Money.split(money, parts)
  [dividend | split(Money.sub!(money, dividend), parts - 1)]
end

[jdewar]

@Wigny If I had thought of this before I posted I would've implemented it, bragged about its elegance, and never looked back. Thank you. Now that I've looked into the problem as much as I have I'm stuck thinking about a more complete solution, and CPU cycles.

@coladarci I've been focused on a line item - a struct with a unit price and quantity - and I apply a discount to the whole line item; now I want to know how much is each unit is worth (useful when refunding after a return). You've made me realize I'm narrowly solving the 'split into even parts' where one might want to split into various proportions like what you've built out.

ex_money_split.livemd.md

I've attached a .livemd (with an extra .md to get it to upload) that runs ExUnit tests with StreamData:

1. Money.split as is currently
2. Money.split using :rounding_mode as I started this issue with, but actually doing it in the right spot
3. Wigny's solution, just to prove out that it works

Please point out any weaknesses in the tests

[jdewar]

After a detour into apportionment (like, how representatives are assigned to voting districts) I see that spreading discrete values based on a percentage is not a trivial thing, maybe 'unsolvable'? ie. there's no strong stability to any solution (eg. if you add votes to the total, you might lose a representative.)

Partially the issue/opportunity is to decide when the split occurs - before the initial rounding to the currency’s precision or after.

Do either of you have a strong view on this?

My input value being rounded w/o opts being passed in felt wrong, but after spending an obscene amount of time on this, I realized that to expect rounded amounts as an output you need the input amount to be boring; my view is it's good to put a default rounding, for the reasons you said here:

The reason that the first rounding happens is that the amount can be arbitrary precision. And therefore it needs to be rounded to the precision of the currency before splitting. This is a design choice of course - the assumption being that Money.split/2 is going to be used to allocate real money and therefore triggers rounding - also so we can calculate the remainder.

---

I would like to keep the default rounding where possible. So I'm thinking to apply that first, and only if the remainder is negative recalculate using :down. This should result in the largest possible factor with the smallest possible remainder. An assertion that needs testing of course, and I'll do some property tests as well.

The above linked .livemd.md file is a Livebook file that had to be named .md to be uploaded. The second ExUnit.run() uses the :down rounding in the right spot, I believe.