Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiplication of per units #32

Open
maennchen opened this issue Feb 10, 2023 · 14 comments
Open

Multiplication of per units #32

maennchen opened this issue Feb 10, 2023 · 14 comments

Comments

@maennchen
Copy link

It would be nice if composite (per) units could be multiplied with its base units.

For example:

drain_rate = Cldr.Unit.new!("liter_per_hour", 6)
time = Cldr.Unit.new!("minute", 30)
Cldr.Unit.mult(drain_rate, time)
# => #Cldr.Unit<"liter", 3>
@kipcole9
Copy link
Collaborator

Great suggestion. So basically allow "per" unit to be multiplied by either its numerator base unit or its denominator base unit since these are algebraically straight forward.

That part of the code is somewhat complex so it might take me a little while to implement but I should have it done by the end of the weekend.

@maennchen
Copy link
Author

Yes, exactly that.

This was just something like I stumbled upon when trying to refactor some code. I have a working solution right now, so this is not at all something pressing. Just a good idea I think.

Take your time and let me know if I can be of help 😊

@kipcole9
Copy link
Collaborator

Not forgotten, just a bit time-challenged a the moment.

@kipcole9
Copy link
Collaborator

kipcole9 commented Mar 7, 2023

Back on this issue this week now that I've cleared some backlog items. Will try hard to have this resolved by weeks end.

@kipcole9
Copy link
Collaborator

kipcole9 commented Mar 9, 2023

I've pushed two commits that have an initial implementation:

The work is not complete however. Using your example:

iex> drain_rate = Cldr.Unit.new!("liter_per_hour", 6)
Cldr.Unit.new!("liter_per_hour", 6)
iex> time = Cldr.Unit.new!("minute", 30)
Cldr.Unit.new!(:minute, 30)
iex> Cldr.Unit.mult(drain_rate, time)
Cldr.Unit.new!("liter_minute_per_hour", 180)

You can see that the factors aren't being reduced. I have an implementation to reduce factors but it resulted in a quite large number of test failures on the standard CLDR test data suite so I need to revisit that part next.

Comments and suggestions are of course welcome.

@maennchen
Copy link
Author

maennchen commented Mar 9, 2023

@kipcole9 That is awesome. ❤️

So the missing piece if I understand correctly is that we have to identify if there are compatible units in the dividend and the divisor. If there are, we eliminate them from both.

Therefore $\frac{6\text{ liter}\cdot30\text{ minute}}{1\text{ hour}}$ would be simplified to $6\text{ liter}\cdot\frac{30\text {minute}}{1\text{ hour}}$, $6\text{ liter}\cdot\frac{30\text {minute}}{60\text{ minute}}$, $6\text{ liter}\cdot\frac{1}{2}$ and then to $3\text{ liter}$.

How do we represent divisions that do not work clean? Just as a ratio?

drain_rate = Cldr.Unit.new!("liter_per_hour", 1)
time = Cldr.Unit.new!("minute", 17)
Cldr.Unit.mult(drain_rate, time)
# => Cldr.Unit.new!("liter", 60 <|> 17)

We could probably approach it something like this (pseudo-code):

simplified_units = for %Cldr.Unit{amount = dividend_amount, unit: dividend_unit} = dividend <- dividends(input),
  divisor <- divisors(input),
  Cldr.Unit.compatible?(dividend, divisor),
  # Careful, this pseudo code ignores that each divisor / dividend must only be used once...
  reduce: input do
  acc ->
    %Cldr.Unit{amount = divisor_amount} = Cldr.Unit.convert!(divisor, dividend_unit)

    acc
    |> remove_dividend(dividend_unit)
    |> remove_divisor(divisor_unit)
    |> mult(dividend_amount <|> divisor_amount)
end

# Cldr.Unit.new!(x, 7 <|> 1) => Cldr.Unit.new!(x, 7)
simplified = case simplified_units do
  %Cldr.Unit{value: %Ratio{denominator: 1, numerator: value}} -> %Cldr.Unit{simplified_units | value: value}}
  _other -> simplified_units
end

The same applies to multiplications as well I guess. For example $6\text{ meter}\cdot50\text{ centimeter}$ should probably result in $3\text{ square meter}$.

In extreme cases we also might have $\frac{6\text{ square meter}}{3\text{ meter}}$, which should result in $2\text{ meter}$.

@maennchen
Copy link
Author

Thinking about it a bit more, we probably have to take a less naive approach to solve those.

GNU Units does what we're looking for:

units "8meter/second * 300second * 5meter / 10 centimeter"
# => 120000 m

Maybe we could get some inspiration from there...

@kipcole9
Copy link
Collaborator

kipcole9 commented Mar 9, 2023

Very doable. I have most of this implemented but not wired up (due to the unexplained test errors). I can wire it up for only this mult/div use case and see where that gets us. I can do some more work on this tonight my time (back in Singapore). Great hint about units, I always forget about that.

@kipcole9
Copy link
Collaborator

kipcole9 commented Mar 9, 2023

And I probably should built a units semi-clone as a good test harness if nothing else.

@kipcole9
Copy link
Collaborator

Apologies for taking so long on this. The cycle for CLDR 43 updates was longer than I planned or expected. I am nearly finished now on adding localized verified routes to ex_cldr_routes and getting back to this issue is next on the list.

@bolte-17
Copy link

This seems like it's the same root problem I immediately ran across when testing out this library.

First thing I tried out was some very basic multiplications/divisions and the results were both extremely surprising and made the library not useful for my intended purpose.

Basic examples were
(1 :meter) * (1 :meter) returns 1 :meter rather than 1 :meter^2 as expected.
(1 :meter) / (1 :meter) returns 1 :meter rather than 1 as expected.

The docs might be implying there's a difference between div!/2 and div/2 here in that the first always returns the first argument's unit, but I'm not sure that's intended. Behavior doesn't seem to differ in that regard between the two functions.

I'm slightly confused if there's even a means to represent dimensionless quantities in this library. The configuration of new units seems to imply :unit is a thing but that's not a valid atom for Unit.new. I'd expect it to be valid to div and mult a dimensioned unit with a scalar, i.e. (5 meters) * 10 => (50 meters).

@kipcole9
Copy link
Collaborator

All good points. This library started primary to provide unit localisation and conversion. And then I started work on basic math and algebra which clearly isn't correct of complete. And the last major refactor for the lib - which focused on conversion precision - drained me. Its time I got back to this issue and addressing your comments.

  • Scalar mult/div I can take care of in the next 24 hours
  • Fixing the basic mult/div to correct common factors I should also be able to fix in the next 24 hours
  • Dimensionless units I need to revisit. I had a good reason why its in CLDR but not supported in the lib. I just don't recall what that was. I think it may have been because there is no localisation/translation for unit. I'll look into this but maybe not until the weekend.

Your reigniting this issue is good motivation for me so hopefully you might care to collaborate and keep pushing. And if you can explain your use case and requirements I can keep focused on what you need for the next iteration.

@bolte-17
Copy link

If you're feeling motivated that's great, but there's no rush. I was looking around for libraries mostly to put some guardrails on a bunch of durations we're currently passing around as just raw numbers, sometimes representing hours, sometimes representing seconds, etc. We also have some domain-specific units of time like treating 15 minutes as a single "unit". It looked like I could try this library out for that purpose, since it supported custom units in a way that I wouldn't have to redefine every single time unit conversion manually.

Was thinking of doing things like "round to nearest 15 minutes", with the 15 minutes being somewhat configurable, but taking an actual quantity with units rather than just an integer and then having to guess whether it was a number of hours or seconds. Could implement as something like

def nearest_multiple(quantity, unit) do
  quantity |> div(unit) |> round() |> mult(unit)
end

This library might not end up being the right tool for the job anyway- I see now how this is more focused on localization- but it seemed like those mult/div results were so surprising I should at least put out an issue or comment.

@kipcole9
Copy link
Collaborator

It's been nearly a year but I'm back on this issue. I've had to do quite a lot of refactoring for other CLDR-related reasons but thats now done. I've added support for multiplying/dividing a unit by a scalar value (phase 1). Now I can work on multiplying and dividing "incompatible" units and then doing common factor reduction.

I'm aiming to get this done in time for a release cycle in April that coincides with CLDR 45.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants