Does Prefix need a UnitConverter?

[keilw]

Based on discussions in https://github.com/unitsofmeasurement/unit-api/issues/85 and hints, we may consider offering default prefixes like MetricPrefixor BinaryPrefix in the API, what should make up the Prefix?

Currently Prefix among String methods declares

/**
   * Returns the corresponding {@link UnitConverter}.
   *
   * @return the unit converter.
   */
  public UnitConverter getConverter();

While the implementations of UnitConverter like RationalConverter or MultiplyConverter are not exactly rocket-science, pulling them into the API would blow it up quite significantly. And although V2 aims at Java SE we should not outrule very special converters in specific environments.

So the question is, does the Prefix need a UnitConverter or would a member variable like value or factor be sufficient?

The old BinaryPrefix in uom-lib-common retrofitted as an enum now, too shows how this may look like in the API if we went for it. I am not casting a vote yet, I am unbiased and could see it in either place.

[keilw]

A) Yes

[keilw]

B) No, a factor seems enough.

[desruisseaux]

I'm neutral on this question. The use of UnitConverter here is not necessarily an issue; it does not prevent users to use Unit-API in an implementation-independent way. It may prevent mixing two independent implementations, but this is a separated issue. Mixing two implementations is already not well supported, as discussed on units-dev@g….com mailing list on March 9th.

[keilw]

Since a call to unit.multiply(factor) also produces an implementation of Unit, even the factor may not guarantee 100% reusability across multiple implementations, but it sounds appealing to define some of the most basic and widely-used items like SI prefix in the API if we can. Please note, if we did go fo B and defined MetricPrefix, potentially also BinaryPrefix (open to discussion for that) as "hybrid enums" then offering a default method return Collections.<Prefix>unmodifiableSet(EnumSet.allOf(prefixType.asSubclass(Enum.class))); is a logical result. It makes no sense to declare one or two default prefixes in the API but split that simple line into the RI. If we use enums as prefixes in the API, then it should be in the API as well. A defaultmethod is no final, so every implementation may override it if they use something else.

[keilw]

@unitsofmeasurement/experts and contributors like @andi-huber, @filipvanlaenen or @ejberry (especially if the approach had an effect on JVM language bindings like Kotlin) would you mind casting a few votes on this? Thanks.

[desruisseaux]

Alternatively, no method in Prefix and specialized methods in sub-types only:

• getPowerOf10() in MetricPrefix
• getPowerOf2() in BinaryPrefix

Compared to factor, powerOfXXX() avoid rounding errors for very large factors or for any factor smaller than 1.

[keilw]

Keep in mind we have the methods like KILO() (in the RI) or KIBI() (currently in the common library) so we must remain backward compatible. The only alternative if everyone disagreed on that (currently here we also have 2 votes for a getValue() or getFactor() approach similar to UCAR Prefix) was to drop the idea of Prefix in the API entirely and have implementation-specific approaches that may either use UnitConverter, a numeric factor or a combination of both. So just in case there was a huge majority towards not standardizing the Prefix at all I add option:

[keilw]

C) Do not standardize Prefix at all and leave the current prefix classes like hey are in the RI or other places.

[desruisseaux]

Not sure what is the compatibility issue with KILO, etc. methods in existing implementation? You want the RI to compile without any change?

[keilw]

Although the package name is different yes. And people expect something like KILO(GRAM) or kilo(GRAM) - we picked uppercase because the method represents a constant factor - not getPowerOf10(8, GRAM) or similar, that would just feel totally odd and alienate users. Even quantity which is a "marker interface" also has a couple of methods, thus a Prefix that is no more than a tagging interface feels wrong. And we might as well drop the idea of standardizing it alltogether. Hence option C for those who think it should not be standardized.

While I haven't been in an EG call for JSR 382 in a while (now the client does not need configuration on the JVM but other things like API security) it seems the JSR also has many similar detail questions to be discussed. However being a 2.x release the backward compatibility is important here. If introducing a Prefix was to break existing code rather than add a new feature, then let's abandon the idea and focus o other topics.

[keilw]

Right now one option of https://github.com/unitsofmeasurement/unit-api/issues/85 has a majority (even if we counted multiple up- or down-votes instead of just one per participant) but let's try to answer this, because option C here would make https://github.com/unitsofmeasurement/unit-api/issues/85 completely irrelevant. And others e.g. allowing to declare it in the API here would also influence which alternative is viable or not.

[desruisseaux]

I agree that users should be able to recompile their code with no change, but is it really required for implementors? Not all standards have this requirement (e.g. JDBC, JDK tools).

Anyway, Prefix was not part of previous Unit-API. So even implementations will not have anything broken by the proposed alternatives.

[keilw]

Yeah but that's not the point, a method like toThePowerOf10() sounds too strange and mathematical. Those who were in JSR 275 remember when folks like Josh Bloch also voted against it because aspects of it like "SI" sounded too much like "Sheldon Cooper" to them and no what they expected in every day usage. Even while getting some inspiration from BigDecimal and the JDK we may not want to introduce methods likeulp()here. Some JSRs like Servlet have a few default classes or even a few more. Others like CDI or JSON-P have just accessor and service loader facades and no other concrete classes.

[dautelle]

Following my previous comment on #85 I would recommend not having a Prefix class in the API but two utility classes "Metric" and "Binary". It is expected that prefixes will be used through static import, e.g. import static javax.measure.Metric.*; Unit<Length> km = KILO(Meter);

[keilw]

@dautelle Thanks, if we restrict them to using multiply like JSR 108 did, or the uom-lib-common binary prefix, then it could work to keep those in he API. Would you vote for C) here then, it suggests not declaring the Prefix and leave or put concrete classes elsewhere (those appropriate may also be in he API)

[dautelle]

Yes, but I would recommend adding to the API both "Metric" and "Binary" utility classes (with static methods only).

[filipvanlaenen]

The thing that I reacted to the most was that you're returning a UnitConverter as a response to a getConverter() method on a Prefix object. I would expect a PrefixConverter :-)

Maybe this is an indication that the naming for the UnitConverter class is wrong. I checked its definition, and it doesn't seem to be related to Unit at all. While we're at it, I had wished there was a compareTo method on the (Unit)Converter class, so I could figure which Converter/Unit/Prefix was larger (e.g. while trying to implement addition).

[keilw]

How would a PrefixConverter be different from a UnitConverter to justify yet another API or SPI element? The name historically started with JSR 275 (108 only called it something like Converter AFAIR) and to be honest, it is a little bit misleading because the result of the conversion is a number, not a Unit. It does however convert those numbers from one Unit to another, so in the end it's not that wrong and part of the 1.0 standard so it'll likely stick around for some time.

[filipvanlaenen]

The name UnitConverter starts to become wrong when it is also used to convert Prefixes. But it's not a big deal.

About the actual issue, I've voted for option B now.

[keilw]

Because it converts numbers from one Unit to another it worked well to get that numerical factor, but if we're able to use the factor directly for all prefixes that we imagine useful, then it is not necessary here. Thanks for voting. I copy/move the two default prefixes to the SPI package. Let's take it from there, at the moment option B (keep the Prefix interface but replace UnitConverter with a numeric value/factor) has a solid majority.

[andi-huber]

When replacing UnitConverter with a numeric value/factor I'd suggest to have a closer look at what this factor really is:

1. For MetricPrefix and BinaryPrefix it is BASE^POWER with both BASE and POWER being (mathematically speaking) integers. So the prefix factor can be defined precise, without resorting to a floating point representation. That way also ONE (the multiplicative identity) is very well defined.
2. In rare cases a floating-point representation might be required.

Any implementing library that does Prefix calculus (eg. multiplication) will in case of 1) when having access to BASE and POWER be able to do this with arbitrary precision.

[andi-huber]

So I'd propose this as most likely sufficient:

public interface Prefix {
  public String getSymbol();
  public int getBase();
  public int getPower();
}

[keilw]

BigDecimal or BigInteger have the a pow() method, so with a Number factor (maybe "value" is the better name) it holds something like TEN.pow(24). We are restricted to the algorithmic operations of Unit, which also has a pow() method, but that won't take the base into consideration. BigDecimal or BigInteger are fairly precise, shouldn't that be sufficient or am I missing something?

Being able to represent both cases was a reason why UnitConverter got applied. It allows multiplication by a floating point value like double or BigDecimal as well as the RationalConverter which contains the base and power element, but directly on the Unit. Actually BigDecimal or BigInteger did not work, but Math.pow() seems to work, have to compare the API level prefix with the ones before in Indriya, there JUnit tests should demonstrate if the factor works or not.

It seems calling it Factor was maybe not such a bad guess. https://en.wikipedia.org/wiki/Metric_prefix contains a column with a Power operation but also the numeric factor that's a result of that operation, so storing a number might be enough.

I'm afraid however, RationalConverter in the RI or other implementations is not so easy to replace. After @filipvanlaenen added a patch to Indriya certain multiply operations of a Unit automatically result in the RationalConverter while other values get handled by MultiplyConverter. Especially the biggest prefixes exceed Long, so they do not fall in the RationalConverter automatism. I am not sure, if it'll work without the prefix taking care of the UnitConverter.

[andi-huber]

What if we add one operation to Unit (and Quantity) such that

import static javax.measure.test.unit.DistanceUnit.m;
...
final Unit<Length> km = m.transform(KILO.getConverter());

becomes

import static javax.measure.test.unit.DistanceUnit.m;
...
final Unit<Length> km = m.prefix(KILO);

where the implementation takes care of the prefix calculus?

Redefine Prefix

public interface Prefix {

    /**
     * Returns the symbol of this prefix.
     *
     * @return this prefix symbol, not {@code null}.
     */
    public String getSymbol();

    /**
     * Base part of the associated factor in base^exponent representation.
     */
    public int getBase();

    /**
     * Exponent part of the associated factor in base^exponent representation.
     */
    public int getExponent();

}

Extending Unit

public interface Unit<Q extends Quantity<Q>> {

  ...

  Unit<Q> prefix(Prefix prefix);
}

Implementing Prefix

public enum TestPrefix implements Prefix {
    MEGA("M", 10, 6),
    KILO("k", 10, 3);

    private final String symbol;
    private final int base;
    private final int exponent;

    /**
     * Creates a new prefix.
     *
     * @param symbol
     *          the symbol of this prefix.
     * @param base
     *          part of the associated factor in base^exponent representation.
     * @param exponent
     *          part of the associated factor in base^exponent representation.
     */
    private TestPrefix(String symbol, int base, int exponent) {
        this.symbol = symbol;
        this.base = base;
        this.exponent= exponent;
    }

    ...
}

Implementing Unit

public abstract class TestUnit<Q extends Quantity<Q>> implements Unit<Q> {

    ...

    public Unit<Q> prefix(Prefix prefix) {
        final MultiplyConverter converter = 
                new MultiplyConverter(Math.pow(prefix.getBase(), prefix.getExponent()));
        return this.transform(converter);
    }
}

[keilw]

@unitsofmeasurement/experts @filipvanlaenen @andi-huber Hi, please have a look at the 7 broken JUnit tests Indriya still has after the change to a numeric factor: https://circleci.com/gh/unitsofmeasurement/indriya/405

Especially

 PrefixTest.testNano:137 expected: <1.0E9> but was: <9.999999999999999E8>
  PrefixTest.testNano2:148 expected: <1.0E9> but was: <9.999999999999999E8>
  PrefixTest.testSingleOperation:161 expected: <µg> but was: <g/1000000.0>

All related to NANO conversion look like a precision issue. Since the prefix() operation @andi-huber proposed above has the same result as the pow() inside MetricPrefix at the moment, it would not make a difference if we declare it as base and exponent. It may help to somehow apply the same RationalConverter as the old MetricPrefix did underneath the hood.

[keilw]

Except for two (that depend on BinaryPrefix from the SPI) all others work after I added the original RationalConverter back to the RI MetricPrefix. It is the only converter that works with the necessary precision. Whether the Unit.prefix() idea is feasible I can't say, because it would force all operations to change to a "reverse notation" of GRAM.prefix(KILO) instead of KILO(GRAM). I cannot say, if this is a good idea and we also have to make sure that life and usually rather natural expressions in JVM languages like Kotlin or Groovy are not broken or turned into a weird unexpected form.

Actually it may work if we wrapped such a new or updated operation like Unit<Q> prefix(Prefix prefix) or even call it Unit<Q> transform(Prefix prefix) inside a concrete prefix:

  public static <Q extends Quantity<Q>> Unit<Q> KILO(Unit<Q> unit) {
    return unit.prefix(KILO);
  }

or unit.transform(KILO) whatever we might call it.

What had to happen as a logical result is, that javax.measure.spi.Prefix became javax.measure.Prefix because Unit must not have dependencies on something in an optional package like SPI.

[andi-huber]

I've created forks of unit-api [1] and indriya [2] to demonstrate, how proposed base/exponent implementation could look like. I also introduced a new UnitConverter namely PowerConverter [3] specifically to do prefix calculus.

[1] https://github.com/andi-huber/unit-api [2] https://github.com/andi-huber/indriya [3] https://github.com/andi-huber/indriya/blob/master/src/main/java/tech/units/indriya/function/PowerConverter.java

[keilw]

Hi @andi-huber, Thanks. Do they all pass JUnit tests like in the master branch right now? (actually the 2 disabled ones for BinaryPrefix should also pass if the new converter works as intended)

If they do, why don't you propose a PR? I know it has dependencies on the API so please propose both.

One thing in the API, please put Prefix into javax.measure, not just the two concrete enums. As you introduced a dependency to Prefix in Unit it must be in the same top level package. Similar to default implementations of e.g. javax.servlet.Servlet these classes make most sense in the core module and package as well. In this case compatible implementations MAY add further prefixes, but those that are fine with MetricPrefix won't have to implement the interface.

[andi-huber]

Thanks @keilw ! I'm still working on 15 JUnit Tests failing. These seem String Formatting related.

If I manage to get these fixed, I'll propose a PR.

[keilw]

Great, thanks a lot for your help. If any of those did not work, a "dividend/divisor" approach for RationalConverter would because in master (with patch of MetricPrefix) all relevant tests are green.

[andi-huber]

I managed to get all JUnit Tests positive, except for one I have disabled now: PrefixTest.testSingleOperation(): both Units have different UnitConverters, check for equality will always fail; is related to PrefixTest.testNestedOperationsShouldBeSame() also being disabled.

Today was my first look at indriya and I think the UnitConverter's composition mechanics needs some robust redesign. But I guess this is related to this discussion at: https://github.com/unitsofmeasurement/uom-se/issues/164

Also I marked AbstractUnits.IDENTITY deprecated to double check, whether its always used correctly.

Other than that great work everyone!

[desruisseaux]

@andi-huber is right, the use of base and exponent allow accurate arithmetics where the use of double is subject to rounding errors. Andi's proposal is same than mine and for the same reasons, except that he declares the base and exponent as two separated properties while my proposal has the base implied by the Prefix subtype (all metric prefixes use base 10; all binary prefixes use base 2). I have no strong preference between those two alternatives.

I would not promote the use of BigDecimal as a replacement for (base, exponent) properties. Knowing the base and exponent give more possibilities to perform efficient and accurate arithmetic, and those information are not easy to guess from a BigDecimal. On the other side, it is much easier to compute a BigDecimal from a base and exponent if someone wants to.

[keilw]

I'm not sure about BigDecimal (could a base or exponent be a floating point number) but I considered BigInteger instead of int or at least long. Trying to calculate a single factor in the API already turned out to be tricky, I also tried a single factor, but it had side-effects on existing converters in the RI, so @andi-huber's approach harmonized better with those.

[desruisseaux]

@andi-huber's approach is fundamentally same as mine, that you rejected earlier in this thread (but admittedly Andi did the right thing: providing code to prove that the base + exponent approach is a good way to work).

In the MetricPrefix class, the base field can be removed and the getBase() method can return the hard-coded 10 value. This is what was implied by my getPowerOf10() method (but Andi's way is all right - I'm not asking for change)

In the BinaryPrefix class, the base field can be removed and the getBase() method can return the hard-coded 1024 value. This is what was implied (with a different base) by my getPowerOf2() method. The method naming may not have been ideal, but I have no strong opinion on those one - the important thing is the base (implied or explicit) + exponent (or power) approach.

The use of BigInteger instead of BigDecimal does not improve anything; the same objections than for BigDecimal still apply (hard to guess what was the original base and exponent).

[keilw]

The special names for things like powerOf10() vs. others is what I found unpractical. A little bit like the many ofMillis(), ofDecades() etc. in JodaTime/JSR 310. Since base and exponent together create the much larger number, I guess leaving them int or double (at least Math.pow() has double arguments) looks sufficient there.

[desruisseaux]

Yes of course having base and exponent as int is sufficient.