unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Units.KILOMETRE is referenced in JavaDoc #370

Closed alan-czajkowski closed 2 years ago

alan-czajkowski commented 2 years ago

Units.KILOMETRE is referenced as an existing unit here: https://github.com/unitsofmeasurement/indriya/blob/master/src/main/java/tech/units/indriya/unit/Units.java#L482

but does not actually exist

probably need to define something like:

public static final Unit<Length> KILOMETRE = KILO(METRE);
keilw commented 2 years ago

Yes it's a minor issue about the documentation, will fix it thanks for spotting, but unlike other unit systems (including https://github.com/unitsofmeasurement/uom-systems/tree/master/unicode) where these libraries (in this case ICU4J) we do not expose all the possible combinations of prefixes here, so if you want a KILOMETER or KILOMETRE you'd have to use CLDR or define it in your own app.

alan-czajkowski commented 2 years ago

I don't think asking for a KILOMETRE is too much to ask, it is a very popular unit, unlike other *metre units, and if both GRAM and KILOGRAM are defined in the library it is safe to also define METRE and KILOMETRE (using the same logic/justification: popularity, etc.)

I also found this bug with GRAM:

System.out.println(GRAM);
System.out.println(GRAM.getName());
System.out.println(KILOGRAM);
System.out.println(KILOGRAM.getName());

produces:

g
null
kg
Kilogram

and this bug:

public static final Unit<Length> KILOMETRE = KILO(METRE);
...
System.out.println(METRE);
System.out.println(METRE.getName());
System.out.println(KILOMETRE);
System.out.println(KILOMETRE.getName());  

produces:

m
Metre
km
null

KILO should act as a modifier for both GRAM and METRE and when printing the name, it should default to prefixing the original name with Kilo in front, so in the Metre case the name should print as Kilometre (by default)

similarly for GRAM, in the library KILOGRAM is the default unit and GRAM is defined as:

public static final Unit<Mass> KILOGRAM = addUnit(new BaseUnit<Mass>("kg", "Kilogram", UnitDimension.MASS), Mass.class);
...
public static final Unit<Mass> GRAM = addUnit(KILOGRAM.divide(1000));

but it would make more sense to make GRAM the original unit, and then define KILOGRAM = KILO(GRAM), and ensure that the name gets the proper Kilo prefix:

public static final Unit<Mass> GRAM = addUnit(new BaseUnit<Mass>("g", "Gram", UnitDimension.MASS), Mass.class);
...
public static final Unit<Mass> KILOGRAM = addUnit(KILO(GRAM));
keilw commented 2 years ago

We might consider it in systems-unicode where ICU4J also exposes lots of prefixes like CENTILITER or GIGAHERTZ, but we won't do it here.

alan-czajkowski commented 2 years ago

I really think my explanation and justification above warrant implementing the suggestions (they make sense).

What about the bugs I mentioned?

andi-huber commented 2 years ago

The kilogram is the base unit of mass in the International System of Units (SI). Hence we have it in Indriya. But as Werner mentioned, we don't have Kilometre or other/similar prefixed units exposed as predefined constants. (We could but I guess finally decided not to.)

As for the Unit.getName() method: Seems to regularly cause some confusion. If I understand correctly, it is not recommended to be used by consumers of the library at all. Use the various formatting options instead. (Javadoc is also rather vague on the Unit.getName() method.)

alan-czajkowski commented 2 years ago

@andi-huber I don't think the international standards affect the implementation of the library :) I understand that kilogram is the base unit in SI, but that does not mean it should not be implemented correctly within the library itself: it should be defined as:

public static final Unit<Mass> KILOGRAM = addUnit(KILO(GRAM));

instead of:

public static final Unit<Mass> KILOGRAM = addUnit(new BaseUnit<Mass>("kg", "Kilogram", UnitDimension.MASS), Mass.class);

and this would still satisfy the desire to have the international base unit pre-defined in the library :) I think correctness should be the motivation here

andi-huber commented 2 years ago

If that works, I have no objection implementing it that way. I'll have a closer look later today or tomorrow.

keilw commented 2 years ago

@alan-czajkowski In case of KILOGRAM that would be wrong, it is perfectly fine

public static final Unit<Mass> KILOGRAM = addUnit(new BaseUnit<Mass>("kg", "Kilogram", UnitDimension.MASS), Mass.class);

The kilogram is a base unit and the proposed declaration would destroy that, so @andi-huber please do not change anything there. GRAM is the only exception, strictly speaking it should not be there but UCUM also defines it (see https://ucum.org/ucum.html#section-Base-Units that is a breach with the SI System and Indriya is consistent with SI here, while offering the bridge to UCUM here. Everything else is not declared in UCUM either, there are no constants like MILLIMETER, MILLIGRAM, etc. that is why we should not clutter the RI because the prefixes exist in the API and may be combined with every given unit. ICU4J is redundant because it also added a MeasurePrefix enum a lot later, but that cannot be directly combined with the units, therefore it defined the likes of MILLILITER or MILLIMETER, but it only contains OHM and not MILLIOHM although there are measuring devices going down to that or even MICROOHM.

alan-czajkowski commented 2 years ago

@keilw your mind is still stuck on the idea that the SI standards drive the underlying library implementation ... but these are completely distinct concerns that do not need to be mixed ... pre-defining GRAM inside of the library, in order to correctly use the KILO() modifier, does not violate your desire to have the library have pre-defined standard base units the fact that the library can have both GRAM and KILOGRAM = KILO(GRAM) where GRAM is an "extra" (non-standard) unit pre-defined in the library does no harm to the library whatsoever the only result that comes out of this is: the library becomes "better"

keilw commented 2 years ago

@alan-czajkowski I know a bit about the library implementation as do others including @desruisseaux or @dautelle and more recently @andi-huber and whether a unit is a BaseUnit or a different type like ProductUnit (which KILOGRAM = KILO(GRAM) would make it) makes a big difference. And changing that would break lots of things especially the TCK.

keilw commented 2 years ago

I also changed the title, the reason this ticket is open is just to clean that JavaDoc leftover (from probatly JSR 275 times) thus @andi-huber or others please feel free to fix the JavaDoc glitch, but do not tamper with the definition of BaseUnits

desruisseaux commented 2 years ago

Indeed on a conceptual point of view, KILOGRAM is a base unit. It may be tempting to deviate from the international standard for implementation convenience, but while it brings internal consistency (i.e. consistency of the library with itself) it would also brings external inconsistency (i.e. inconsistency with the world outside the implementation). Base units play a role not only for reading/writing unit symbols, but also in domains like dimensional analysis. So I think that "cheating" there would be the kind of risk that the JSR project wanted to avoid.

However it should be possible to keep base units standard compliant, and still have expressions such as KILO(GRAM) == KILOGRAM to behave as expected (maybe it is already the case? I didn't tried). It is harder on an implementation point of view, but it should be possible to have the implementation do some magic if necessary. So maybe we could rephrase this issue as what could be improved without deviating from the international standard?

keilw commented 2 years ago

We have an expression of assertTrue(KILO(GRAM).isEquivalentTo(KILOGRAM)) Because of Java's strict type-safety the notion of "equivalence" was introduced so it also works the same for "1000 g" or "1000 m" compared to KILO(METRE).

keilw commented 2 years ago

Thanks for the input everyone. I fixed the JavaDoc and will close this ticket now. Regarding the getName() method there were also several hints how to use it. Generally every BaseUnit is supposed to have a name and symbol, while transformed or otherwise derived units often don't have a symbol by definition. Their visual representation is influenced via the label (in UnitFormat). Both symbol and name are immutable on an API level, so the only way to apply a name is the internal "SPI" of AbstractSystemOfUnits.

Many declarations in CLDR and other subclasses of that abstract base class show how to do that. Everyone is welcome to contribute more especially in the open source extension libraries (while here and in other parts of the JSR there is the need to at least join the JCP as an Associate member) or demos.

andi-huber commented 2 years ago

I agree with Werner and Martin and I would not change the declaration of

public static final Unit<Mass> KILOGRAM = addUnit(new BaseUnit<Mass>("kg", "Kilogram", UnitDimension.MASS), Mass.class);

either. Simply because of the very definition of BaseUnit:

This class represents the building blocks on top of which all others physical units are created. Base units are always unscaled SI units.

And indeed kilogram is the unscaled SI unit of mass.