Adding some additional units seems to break some conversions

kerrd89 commented 9 months ago

This may be an issue with my poor understanding. But I was able to break tests tests in the cldr_units library by passing some :additional_units which I believe should be acceptable, based on the documentation.

updating config/test.exs to add a decatherm w/ a base_unit of :therm_us

],
  dtherm_us: [
    base_unit: :therm_us,
    factor: 10,
    offset: 0
  ]

These changes do successfully implement a conversion for :therm_us to :dtherm_us (and likewise) These changes cause these failures. It also breaks the conversion of 1 therm_us to killowatt_hour

correct conversion "29.30011111111111111111111111"

wrong conversion: "2.777777777777777777777777778E-7"


1) test #76 [Decimal] that therm-us converted to kilogram-square-meter-per-square-second is {105480400000.0, 0, 7} (Cldr.Unit.Conversion.Test)
 test/conversion_test.exs:42
 ** (Cldr.Unit.IncompatibleUnitsError) Operations can only be performed between units with the same base unit. Received :therm_us and "kilogram-square-meter-per-square-second"
 code: |> Cldr.Unit.Conversion.convert!(unquote(t.to))
 stacktrace:
   (ex_cldr_units 3.16.3) lib/cldr/unit/conversion.ex:265: Cldr.Unit.Conversion.convert!/2
   test/conversion_test.exs:48: (test)

........................................................................................

2) test #76 [Float] that therm-us converted to kilogram-square-meter-per-square-second is {105480400000.0, 0, 7} (Cldr.Unit.Conversion.Test) test/conversion_test.exs:20 ** (Cldr.Unit.IncompatibleUnitsError) Operations can only be performed between units with the same base unit. Received :therm_us and "kilogram-square-meter-per-square-second" code: |> Cldr.Unit.Conversion.convert!(unquote(t.to)) stacktrace: (ex_cldr_units 3.16.3) lib/cldr/unit/conversion.ex:265: Cldr.Unit.Conversion.convert!/2 test/conversion_test.exs:26: (test)

...................................................................................

3) test #76 that therm-us is convertible to kilogram-square-meter-per-square-second (Cldr.Unit.Conversion.Test) test/conversion_test.exs:7 Assertion with == failed code: assert from == to left: :therm_us right: "kilogram_square_meter_per_square_second" stacktrace: test/conversion_test.exs:10: (test)



 Apologies if this is user error. I am happy to update documentation to prevent any future confusion, if this is a me problem.

 Thank you in advance for your time!

kipcole9 commented 9 months ago

Thanks for the report, it's greatly appreciated. I'm sorry for the inconvenience, it shouldn't be this way. I'll work on this over the weekend and get additional units back on a solid footing.

It's not immediately clear why there are test interdependencies like the ones you are showing.

kipcole9 commented 9 months ago

Not forgotten, and apologies for the slow resolution to this. I'm still perplexed. I'm going to ship a new release today but it only focuses on fixing Elixir 1.16 compiler warning and does not resolve this bug. I didn't want to get your hopes up (just yet).

ahorner commented 9 months ago

I'm uncertain if this is related, but under 3.16.4, I'm seeing the following behavior:

# config/config.exs
config :ex_cldr_units, :additional_units,
  inches_h2o: [
    base_unit: :kilogram_per_meter_square_second,
    factor: 248.84,
    offset: 0,
    sort_before: :all
  ]

# test/app/services/conversions_test.exs
defmodule App.ConversionsTest do
  use ExUnit.Case
  alias App.Conversions

  describe ".for_storage" do
    test "converts passed values to a storage-appropriate unit" do
      assert Conversions.Unit.new!(:inches_h2o, 10.212)
             |> Conversions.Unit.convert!(:inches_h2o)
             |> Conversions.Unit.value() == 10.212
    end
  end
end

  1) test .for_storage converts passed values to a storage-appropriate unit (App.ConversionsTest)
     test/app/services/conversions_test.exs:32
     ** (ArgumentError) implicit conversion of 248.84 to Decimal is not allowed. Use Decimal.from_float/1
     code: |> Conversions.Unit.convert!(:inches_h2o)
     stacktrace:
       (decimal 2.1.1) lib/decimal.ex:1915: Decimal.decimal/1
       (decimal 2.1.1) lib/decimal.ex:996: Decimal.mult/2
       (ex_cldr_units 3.16.4) lib/cldr/unit/conversion.ex:419: Cldr.Unit.Conversion.mult/2
       (ex_cldr_units 3.16.4) lib/cldr/unit/conversion.ex:176: Cldr.Unit.Conversion.convert_to_base/2
       (ex_cldr_units 3.16.4) lib/cldr/unit/conversion.ex:195: Cldr.Unit.Conversion.convert_to_base/2
       (ex_cldr_units 3.16.4) lib/cldr/unit/conversion.ex:157: Cldr.Unit.Conversion.convert/4
       (ex_cldr_units 3.16.4) lib/cldr/unit/conversion.ex:150: Cldr.Unit.Conversion.convert/2
       (ex_cldr_units 3.16.4) lib/cldr/unit/conversion.ex:264: Cldr.Unit.Conversion.convert!/2
       test/app/services/conversions_test.exs:38: (test)

I didn't open this as its own issue, because it seems to fit into the general category of "custom unit conversions not working as expected". It seems like some aggressive value transformation is happening behind the scenes, which in my case is resulting in the custom unit definition breaking on conversion attempts. The key difference in my case is that my factor is a floating-point value, rather than an integer.

kerrd89 commented 9 months ago

I didn't open this as its own issue, because it seems to fit into the general category of "custom unit conversions not working as expected". It seems like some aggressive value transformation is happening behind the scenes, which in my case is resulting in the custom unit definition breaking on conversion attempts. The key difference in my case is that my factor is a floating-point value, rather than an integer.

I believe there is an implementation of factor with a numerator and denominator, which could meet your needs. But I don't think it would work still with this bug, it might just be an issue of the units I am converting around however. I think factor is expected to be an integer, I believe

Example here: https://github.com/elixir-cldr/cldr_units/blob/main/config/test.exs#L25

ahorner commented 9 months ago

@kerrd89: thanks; the fractional factor implementation does indeed work for my case. My concerns are the following:

This was working fine with floating-point values in v3.15.0, and
The typespec for factors in the Conversion module suggests that floats should still be working.

I raised my case in this issue in the hopes that it might help pinpoint the changes that are causing the problem you're encountering, as well.

kipcole9 commented 4 months ago

Sorry for the very long delay in working this more deeply. I've finally finished the fairly major refactor I needed to do (common factor reduction in canonical base name calculation). Now I can focus on open issue here with the objective to have the work done in time for the April release cycle that coincides with CLDR 45.

sachamasry commented 4 months ago

Hi,

I just wanted to add to this issue. I've also added additional units (duration, in my case). They simply failed to work until I read in a forum (https://elixirforum.com/t/how-to-add-custom-measurement-units-with-cldr-unit-additional/58430/4) that ex_cldr_units 3.16.3 fixed this problem.

So, I now have the latest release, 3.16.4, installed and the additional units are recognised. The trouble is that I now see this terrible conversion problem, converting from minutes to hours:

iex [03:34 :: 7] > Cldr.Unit.convert(Klepsidra.Cldr.Unit.new!(:minute, 2), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.0005555555555555555555555555556")}
iex [03:34 :: 8] > Cldr.Unit.convert(Klepsidra.Cldr.Unit.new!(:minute, 6), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.001666666666666666666666666667")}
iex [03:34 :: 9] > Cldr.Unit.convert(Klepsidra.Cldr.Unit.new!(:minute, 12), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.003333333333333333333333333333")}
iex [03:34 :: 10] > Cldr.Unit.convert(Klepsidra.Cldr.Unit.new!(:minute, 30), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.008333333333333333333333333333")}
iex [03:34 :: 11] > Cldr.Unit.convert(Klepsidra.Cldr.Unit.new!(:minute, 45), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.0125")}
iex [03:34 :: 12] > Cldr.Unit.convert(Klepsidra.Cldr.Unit.new!(:minute, 60), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.01666666666666666666666666667")}
iex [03:34 :: 13] > Cldr.Unit.Conversion.convert(Klepsidra.Cldr.Unit.new!(:minute, 60), :hour)
{:ok, Cldr.Unit.new!(:hour, "0.01666666666666666666666666667")}

As you can see, there are two problems here: the conversion is very wrong, and even for simple common denominators, there is always a recurring digit, which is likely to result in incorrect rounding errors, in my case when the time comes to calculate invoicable amounts.

Some more examples:

iex [03:34 :: 18] > Cldr.Unit.convert(Cldr.Unit.new!(:hour, 2), :minute)
{:ok, Cldr.Unit.new!(:minute, 7200)}

I have tried to comment out my additional units and recompile to check whether I can get back to normal behaviour, but it didn't work as my additional units are still recognised, and all the calculations are off as in the examples above.

Please let me know if there's any further testing I can perform to help.

elixir-cldr / cldr_units

Adding some additional units seems to break some conversions #41