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

Liters per 100 km to mpg conversion issue #177

Open
eulogi opened this issue Sep 21, 2020 · 9 comments
Open

Liters per 100 km to mpg conversion issue #177

eulogi opened this issue Sep 21, 2020 · 9 comments

Comments

@eulogi
Copy link

eulogi commented Sep 21, 2020

The conversion from liters per 100 km to miles per gallon is not working as expected. You can execute this JUnit test to observe this issue:

package com.test;

import javax.measure.Unit;
import javax.measure.quantity.Volume;

import org.junit.Assert;
import org.junit.Test;

import systems.uom.quantity.Consumption;
import systems.uom.unicode.CLDR;

/**
 * Test unit conversions.
 */
public class UnitConversionTest {

    @Test
    @SuppressWarnings("unchecked")
    public void testLiterPer100KilometersToMilesPerGallon() {
        // Just comment that the only value that seems to be working fine is 1 liter per 100 km.
        double literPer100Kilometers = 10;
        Unit<Consumption<Volume>> MILE_PER_GALLON = CLDR.MILE_PER_GALLON.asType(Consumption.class);
        double milesPerGallonActual = CLDR.LITER_PER_100KILOMETERS.getConverterTo(MILE_PER_GALLON).convert(literPer100Kilometers);
        double milesPerGallonExpected = (100 * 3.785411784) / (1.609344 * literPer100Kilometers);
        Assert.assertEquals(milesPerGallonExpected, milesPerGallonActual, 0.001);
    }

    @Test
    @SuppressWarnings("unchecked")
    public void testLiterPer100KilometersToMilesPerGallonImperial() {
        double literPer100Kilometers = 10;
        Unit<Consumption<Volume>> MILE_PER_GALLON_IMPERIAL = CLDR.MILE.divide(CLDR.GALLON_IMPERIAL).asType(Consumption.class);
        double milesPerGallonImperialActual = CLDR.LITER_PER_100KILOMETERS.getConverterTo(MILE_PER_GALLON_IMPERIAL).convert(literPer100Kilometers);
        double milesPerGallonImperialExpected = 282.480936332 / literPer100Kilometers;
        Assert.assertEquals(milesPerGallonImperialExpected, milesPerGallonImperialActual, 0.001);
    }
}

Related to unitsofmeasurement/uom-demos#97

keilw added a commit to unitsofmeasurement/si-units that referenced this issue Sep 22, 2020
keilw added a commit to unitsofmeasurement/indriya that referenced this issue Sep 26, 2020
keilw added a commit to unitsofmeasurement/si-units that referenced this issue Sep 28, 2020
keilw added a commit that referenced this issue Sep 28, 2020
keilw added a commit that referenced this issue Oct 4, 2020
177: Liters per 100 km to mpg conversion issue

Task-Url: #177
keilw added a commit to unitsofmeasurement/indriya that referenced this issue Oct 8, 2020
177: Liters per 100 km to mpg conversion issue

Task-Url: unitsofmeasurement/uom-systems#177
@keilw
Copy link
Member

keilw commented Dec 6, 2020

@andi-huber This test is there but disabled as FuelConsumptionTest I think this is related to the CO2CarDemo. Doing this with double may not be optimal but it should nevertheless work, and right now here it shows the same 100 times too big symtom we saw elsewhere.

@andi-huber
Copy link
Member

Was checking out uom-systems (master), but cannot compile ...

[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /~/uom-systems/unicode/src/test/java/systems/uom/unicode/FuelConsumptionTest.java:[62,113] incompatible types: inferred type does not conform to equality constraint(s)
    inferred: systems.uom.quantity.Consumption<javax.measure.quantity.Volume>
    equality constraints(s): systems.uom.quantity.Consumption<javax.measure.quantity.Volume>,systems.uom.quantity.Consumption
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Units of Measurement Systems Parent 2.1-SNAPSHOT:
[INFO] 
[INFO] Units of Measurement Systems Parent ................ SUCCESS [  2.087 s]
[INFO] Units of Measurement Systems Quantities ............ SUCCESS [  3.302 s]
[INFO] Units of Measurement Common Unit Systems ........... SUCCESS [  9.379 s]
[INFO] Units of Measurement Unicode CLDR System ........... FAILURE [  0.223 s]
[INFO] Units of Measurement UCUM System ................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

@keilw
Copy link
Member

keilw commented Dec 6, 2020

Sounds weird, never saw that, which JDK are you using?

@andi-huber
Copy link
Member

a bit outdated ...

mvn -version
Apache Maven 3.6.3
Java version: 1.8.0_192, vendor: Oracle Corporation, runtime: D:\opt\jdk8\jre
Default locale: en_US, platform encoding: UTF-8
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

Which JDK major version do I need?

@keilw
Copy link
Member

keilw commented Dec 6, 2020

Try at least 9. The library should run with Java 8 based on the demos, but building requires Java 9 for most repositories. The Travis matrix uses 9, and 12-16, they all build correctly.

@andi-huber
Copy link
Member

andi-huber commented Dec 6, 2020

In CLDR those 2 definitions are wrong!

    /**Constant for unit of consumption: liter-per-100kilometers*/
    public static final Unit<Consumption<Volume>> LITER_PER_100KILOMETERS = addUnit(
            (KILOMETER.multiply(100)).divide(LITER).asType(Consumption.class));;

    /**Constant for unit of consumption: liter-per-kilometer*/
    public static final Unit<Consumption<Volume>> LITER_PER_KILOMETER = addUnit(
           KILOMETER.divide(LITER).asType(Consumption.class));

The divisions need to be flipped! And then also note: LITER_PER_XXX and MILE_PER_GALLON are not commensurable, so these cannot share the same generic unit type Unit<Consumption<Volume>>. However you like to define Consumption, but one unit has to be the reciprocal of the other. Meaning MILE_PER_GALLON is likely to be considered a 'reciprocal consumption'.

    public static final Unit<Consumption<Volume>> MILE_PER_GALLON = addUnit(
            MILE.divide(GALLON).asType(Consumption.class));

Hope that helps.

@keilw
Copy link
Member

keilw commented Dec 7, 2020

Thanks, do you think we could get to a PR for that? And what about the discussion in unitsofmeasurement/uom-demos#97? If <Consumption<Volume>> is fine here, then the CO2CarDemo and energy domain library should also take that into consideration because it makes no sense to handle this calculation differrently in the CLDR module and elsewhere.

@keilw
Copy link
Member

keilw commented Mar 28, 2021

@andi-huber Any luck building this? I changed the division in two cases and found a good Wikipedia reference: https://en.wikipedia.org/wiki/Fuel_efficiency

So according to that:
LITER_PER_100KILOMETERS is Fuel consumption, I would leave that with the current quantity type Consumption, whereas
LITER_PER_KILOMETER or MILE_PER_GALLON are not defined precisely enough by Unicode CLDR/ICU4J, they are called Fuel economy in above Wikipedia page.

There are terms like Energy efficiency and Fuel Efficiency which brings us back to the original page, so maybe call the Quantity type FuelEfficiency or define a Meta-Quantity like Consumption called Efficiency allowing to define LITER_PER_KILOMETER or MILE_PER_GALLON as. <Efficiency<Volume>>

@keilw keilw added this to the 2.1 milestone Mar 28, 2021
keilw added a commit that referenced this issue Mar 29, 2021
keilw added a commit that referenced this issue Mar 29, 2021
@keilw
Copy link
Member

keilw commented May 21, 2021

There does not seem to be much activity here, so moving it to a later milestone.

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

No branches or pull requests

3 participants