elixir-cldr / cldr_units

Unit formatting (volume, area, length, ...) functions for the Common Locale Data Repository (CLDR)
Other
16 stars 13 forks source link

Energy units have inconsistent base units #27

Closed narrowtux closed 2 years ago

narrowtux commented 2 years ago

When trying to convert joules into kWh, I get this error message:

iex(1)> Cldr.Unit.convert(Cldr.Unit.new!(1, :joule), :kilowatt_hour)
{:error,
 {Cldr.Unit.IncompatibleUnitsError,
  "Operations can only be performed between units with the same base unit. Received :joule and :kilowatt_hour"}}

This is because Cldr.Unit.BaseUnit.canonical_base_unit returns different strings for both of these:

iex(2)> Cldr.Unit.BaseUnit.canonical_base_unit!("joule")
"kilogram_square_meter_per_square_second"
iex(3)> Cldr.Unit.BaseUnit.canonical_base_unit!("kilowatt_hour")
"kilogram_square_meter_second_per_cubic_second"

For Joule, it returns kg*m^2/s^s, and for kWh, it returns kg*m^2*s/s^3. Both are mathematically equal but not as strings, obviously.

I'm not sure if this is an intended feature of the CLDR spec, but it seems very weird to have the canonical_base_unit function return different things for different energy units.

narrowtux commented 2 years ago

I've gone ahead and checked this for all the categories:

%{
  acceleration: [:meter_per_square_second],
  angle: [:revolution],
  area: [:square_meter],
  concentr: [:item, "portion", "kilogram_item_per_kilogram_cubic_meter",
   "item_per_cubic_meter"],
  consumption: ["cubic_meter_per_meter", "meter_per_cubic_meter"],
  digital: [:bit],
  duration: [:year, :second],
  electric: [:ampere, "kilogram_square_meter_per_cubic_second_square_ampere",
   "kilogram_square_meter_per_cubic_second_ampere"],
  energy: ["kilogram_square_meter_per_square_second",
   "kilogram_square_meter_second_per_cubic_second"],
  force: ["kilogram_square_meter_second_per_meter_cubic_second",
   "kilogram_meter_per_square_second"],
  frequency: ["revolution_per_second"],
  graphics: [:pixel, "pixel_per_meter", :em],
  length: [:meter],
  light: [:candela, "candela_square_meter_per_square_meter",
   "candela_per_square_meter", "kilogram_square_meter_per_cubic_second"],
  mass: [:kilogram, "kilogram_square_meter_per_square_second"],
  power: ["kilogram_square_meter_per_cubic_second"],
  pressure: ["kilogram_per_meter_square_second",
   "kilogram_meter_per_square_meter_square_second"],
  speed: [:meter_per_second],
  temperature: [:kelvin],
  torque: ["kilogram_square_meter_per_square_second"],
  volume: [:cubic_meter]
}

For some categories, this makes sense, but I don't think this is true for every one with more than one base unit.

kipcole9 commented 2 years ago

@narrowtux, thanks for the report. The units library has given me the most joy of the whole "family" (numbers comes a close second). But equally the most head spins as well.

Thanks too for diving in to the code, there is a lot going on and at this point you're probably doubting my sanity (and maybe yours).

Definitely a bug on my part. If you look at the two units and the conversion factors you see:

iex> Cldr.Unit.Conversion.base_unit_and_conversion :joule
{:ok, "kilogram_square_meter_per_square_second",
 [
   joule: %Cldr.Unit.Conversion{
     base_unit: [:kilogram_square_meter_per_square_second],
     factor: 1,
     offset: 0
   }
 ]}
iex> Cldr.Unit.Conversion.base_unit_and_conversion :kilowatt_hour
{:ok, "kilogram_square_meter_second_per_cubic_second",
 [
   kilowatt: %Cldr.Unit.Conversion{
     base_unit: [:kilogram_square_meter_per_cubic_second],
     factor: 1000,
     offset: 0
   },
   hour: %Cldr.Unit.Conversion{base_unit: [:second], factor: 3600, offset: 0}
 ]}

And as you say, these are algebraically the same. When it comes to determining convertibility I compare canonical base units as you found. The difference is that CLDR knows explicitly about joule and the data for the base unit :kilogram_square_meter_per_square_second is part of the repository.

The base unit for kilowatt hour is derived. If a unit is not explicitly known to CLDR, then the unit is parsed and broken up into its factors. Then I have to reduce these factors to a base unit string (canonical base unit) in order for comparison to proceed.

I clearly have a bug in reducing the factors and its going to be tricky to resolve because I'll have to refactor each factor until it can be reduced no more and the recombine them. Doable, I will make it happen, but I need to put some more thought into how to do that cleanly.

Probably more than you wanted to know. Thanks for the collaboration, I'll get cracking on improving the base unit resolver.

narrowtux commented 2 years ago

Thanks, I appreciate the rambling :)

kipcole9 commented 2 years ago

As of commit f387156 the base units derivation now reduces common factors and the conversion between joule and kilowatt hour appears to be working:

iex> joule = Cldr.Unit.new! :joule, 1
#Cldr.Unit<:joule, 1>
iex> Cldr.Unit.convert joule, "kilowatt_hour"
{:ok, #Cldr.Unit<:kilowatt_hour, 1 <|> 3600000>}

However there are now failing a few tests so it's not ready for release yet. Nevertheless, if you're inclined to try:

{:ex_cldr_units, github: "elixir-cldr/cldr_units", branch: "reduce_factors"}

in mix.exs should do the trick.

narrowtux commented 2 years ago

Thanks for looking at it so quickly! It works for me.

kipcole9 commented 2 years ago

I have published ex_cldr_units version 3.12.2 that addresses the specific issue raised here. The fix allows for the conversion of units where their base units (in string form) don't match but the units themselves are in the same unit category.

The case of converting joule to kilowatt hour fits the case since their base units are different at the string level. The original fix went the extra step of reducing the algebraic conversion symbolically but this introduced other issues. Net this issue is resolved but there may be other situations where this fix doesn't not work.

The changelog entry reads:

Bug Fixes