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

Support interoperability to other unit systems #402

Open
jpink opened this issue Sep 13, 2023 · 15 comments
Open

Support interoperability to other unit systems #402

jpink opened this issue Sep 13, 2023 · 15 comments
Labels

Comments

@jpink
Copy link

jpink commented Sep 13, 2023

My target is to create "custom" units for electricity prices: €/MWh and c/kWh. I added following classes:

public class CurrencyDimension implements Dimension, Serializable { // Copied from BaseUnit because its final ?!?
    // The symbol is '¤'
} 
public class Money extends AbstractNumberQuantity<Money> { // Copied from NumberQuantity because its final ?!?
    // Uses BigDecimal as a value.
    @Override
    public String toString() {
        // Workaround for tech.units.indriya.format.SimpleUnitFormat$DefaultFormat.format(SimpleUnitFormat.java:907)
        return getValue() + " " + getUnit();
    }
} 
public class CurrencyUnit extends AbstractUnit<Money> { // Again it would be nice to extend BaseUnit
    public final @NonNull Currency currency; // Internally uses java.util.Currency

    @Override
    public String toString() {
        // Workaround for tech.units.indriya.format.SimpleUnitFormat$DefaultFormat.format(SimpleUnitFormat.java:907)
        return getSymbol();
    }
}
public class CurrencyCentUnit extends CurrencyUnit { // Symbol is 'c' and name is currency display name + " cent".
    private final CurrencyUnit baseUnit;
}
public class Monetary extends AbstractSystemOfUnits { } // This seems to be optional

When i try to print quantity i get error:

java.lang.IllegalArgumentException: Cannot format given Object as a Unit
	at tech.units.indriya.format.SimpleUnitFormat$DefaultFormat.format(SimpleUnitFormat.java:907)

That is because my class isn't AlternateUnit or ProductUnit.

  1. Can you support standard Unit and Quantity interfaces?
  2. Can create another system of units and combine those different systems with this library? I think I should. Or should I fork this and do the modifications.
  3. And what benefits you get by marking all classes final?

I'm not planning to do currency exchanges (although its possible use some service). Just playing on same currency and combining to SI system.

@jpink jpink changed the title SupportAdd interoperability to other unit systems Support interoperability to other unit systems Sep 13, 2023
@jpink
Copy link
Author

jpink commented Sep 13, 2023

I tried to create separate system using only javax.measure (not to extend tech.units.indriya.* classes). It is working correctly until I try to combine two systems by ProductUnit.ofQuotient:

EURO.toString() // Prints "€"
EURO.divide(Units.MEGAWATT_HOUR).toString() // Prints "null/MWh"

How can I register my Euro unit to SimpleUnitFormat.unitToName map?

I'm out of ideas. I try to use UoM in openHAB, but in this case it seems impossible to tell the unit for the system. Sadly I have to fall back use only BigDecimals.

@jpink
Copy link
Author

jpink commented Sep 16, 2023

Now I found the way to introduce my units using SimpleUnitFormat.getInstance().label(unit, unit.getSymbol())and prossibly extending classes may also work. Investigating...

@keilw
Copy link
Member

keilw commented Sep 16, 2023

Yes, that's indeed the way. Note, in the most recent Indriya versions, there is SimpleUnitFormat.getInstance() which returns a shared instance across the JVM / application lifetime, while SimpleUnitFormat.getNewInstance() creates a new instance that is local to a particular class or session, where label() is not so persistent. Allowing to adjust it for a particular purpose, e.g. a batch run, reporting or something similar, where you might want special settings that do not affect the rest of your application.

For EBNFUnitFormat it would be a little trickier, because you have to create a SymbolMap first, and then one could also use label() on that instance before creating an EBNFUnitFormat instance with it.
I just created ticket #403 to make that easier.

@jpink
Copy link
Author

jpink commented Sep 17, 2023

I don't know what EBNF means 😕

Now i faced minor issue. Let's say that USD and AUD have the same symbol $. Is there any way to embed some metadata (currency code) in the BaseUnit? The class is final, so I can't extend it. The idea what I got, is to create own dimension for each currency. UnitDimension.parse(char symbol) allows only one character and there is 180 currencies. So maybe I can find free space from Unicode. Or it is possible to extend UnitDimension, but if I understood correctly there shouldn't be multiple dimensions with same symbol? Or can I "force" currency code (three chars) as the dimension symbol?

@keilw
Copy link
Member

keilw commented Dec 6, 2023

I was thinking of a "hybrid" between this JSR and JSR 354 (Money and Currency) for quite a while, but it is a bit complicated.
@dautelle had a money implementation back in the days of JSR 275.

BaseUnit should not be extended because it meets a very special purpose, the SI base units, nothing else.
Not sure, why you would use Dimension for currencies, aren't those units? You might have to play a bit with the label() method of SimpleUnitFormat, but that way USD and AUD could have distinct names but the same label ("$")

@dautelle
Copy link

dautelle commented Dec 7, 2023 via email

@J-N-K
Copy link
Contributor

J-N-K commented Jan 5, 2024

I'm working on a similar problem (openhab/openhab-core#3503). While conversion of currencies (we use symmetrical only, but the issue is similar) works fine, product units (like EUR/kWh) don't work.

The reason is that UnitCompositionHandlerYieldingNormalForm throws an NPE:

java.lang.NullPointerException: no normal-form order defined for class 'org.openhab.core.internal.library.unit.CurrencyConverter'
	at java.util.Objects.requireNonNull(Objects.java:336) ~[?:?]
	at tech.units.indriya.internal.function.simplify.UnitCompositionHandlerYieldingNormalForm.isNormalFormOrderWhenCommutative(UnitCompositionHandlerYieldingNormalForm.java:112) ~[?:?]
	at tech.units.indriya.internal.function.simplify.UnitCompositionHandlerYieldingNormalForm.compose(UnitCompositionHandlerYieldingNormalForm.java:80) ~[?:?]
	at tech.units.indriya.function.AbstractConverter.concatenate(AbstractConverter.java:179) ~[?:?]
	at tech.units.indriya.unit.ProductUnit.getSystemConverter(ProductUnit.java:330) ~[?:?]
	at tech.units.indriya.AbstractUnit.getConverterToAny(AbstractUnit.java:327) ~[?:?]

because it has a fixed set of supported converters and it is not possible to add new converters. The set of converters is retrieved from Calculus.getNormalFormOrder() and initializes and returns Calculus.normalFormOrder which is inaccessible.

For us using a DoubleMultiplyConverter would be fine, but that's also not possible, because in our new class CurrencyUnit we can't instantiate that because all constructors are package private.

@keilw
Copy link
Member

keilw commented Jan 5, 2024

Thanks for mentioning this. I suppose, you do have some sort of CurrencyConverter or similar subclass of AbstractConverter?

The Calculus.normalFormOrder is used in a very isolated way, so @andi-huber do you see any problems adding a method like registerUnitConverter, somewhat similar to setCurrentNumberSystem? And if added, how should it handle the order of new, custom converters? Before or after 99 for AbstractConverter.Pair?

@keilw keilw added the analysis label Jan 5, 2024
@J-N-K
Copy link
Contributor

J-N-K commented Jan 5, 2024

Correct, it's a sub-class of AbstractConverter. I believe the proposed method would also allow for asymmetric converters (if they inherit from AbstractConverter) and thus be useful for @jpink.

What I'm doing now is adding it via reflection (with order 1000), but that is of course a dirty hack.

@jpink
Copy link
Author

jpink commented Jan 6, 2024

BaseUnit should not be extended because it meets a very special purpose, the SI base units, nothing else. Not sure, why you would use Dimension for currencies, aren't those units? You might have to play a bit with the label() method of SimpleUnitFormat, but that way USD and AUD could have distinct names but the same label ("$")

@keilw: I added currency "¤" dimension, because I don't understand the dimension system and don't want to mess with existing ones. $ symbol isn't problem for me for now. I use ISO code for parsing using alias. And currencies are shown using symbols. The user knows the context.

@dautelle: I don't need to exchange currencies. I only want to convert units eg. 100 €/MWh => 10 snt/kWh. That's easy divide by ten without the units library, but I have fetish to use it 😄

For us using a DoubleMultiplyConverter would be fine, but that's also not possible, because in our new class CurrencyUnit we can't instantiate that because all constructors are package private.

@keilw, @J-N-K: I don't understand what are the benefits of closing every single class in the implementation if they aren't the public API. Developers can always fork codes and patch them (like I often do). But that causes that I can't integrate my implementation to OpenHAB. Currently my UoM works as I like inside the binding, but OpenHAB doesn't understand those units. So I use full Quantities inside of the binding and tell only numbers (without units) to OpenHAB.

BaseUnit should not be extended because it meets a very special purpose, the SI base units, nothing else.

@keilw: I really understand if there is good reasons to close something. But then you should document those (in the code) and tell what is the correct way to do things or provide some workaround (in documentation).

Sorry but I got a little pissed of this library. I didn't got it working in three weeks, so I give up, let it be and done thing the other way.

@J-N-K
Copy link
Contributor

J-N-K commented Jan 6, 2024

I would like to ensure that one thing is 100% clear: The statement of @jpink is in no way endorsed by openHAB maintainers group and does not reflect my or any other opinion I know of.

@keilw
Copy link
Member

keilw commented Jan 7, 2024

Thanks for the clarification. The beauty of open standards is, there are multiple implementations, if @jpink prefers to write his own, or use another one, sure no problem.
Will use this ticket to see, how Calculus could be extended for additional converters.

@andi-huber
Copy link
Member

In a broader sense having units for money with different currencies (USD, EUR, ...) falls in the same ballpark as having units like RADIAN and DEGREE. All those units have same dimensions (dimensionless in that case) but cannot necessarily be converted into one another. (I believe you cannot convert RADIAN into USD, or RADIAN int STERADIAN.) We say those units are not commensurable. However having categories of units that form a set of commensurable units, would be nice. E.g. having a set of units for the category money that allows for converters to be defined within this category.

Not sure if the current API can do this or if it can be extended to do this.

I don't have immediate answers. Perhaps I find time later this month.

Some related material:

  1. There can be only one ONE How SystemUnit is figured out? #361
  2. STERADIAN/RADIAN discussion Angular units such as RADIAN and STERADIAN are dimensionless #257
  3. Introduction of Unit Categories Covers some angular use-cases by providing a 'AngularTest' class. #258
  4. Unit Equivalence (Wiki) https://github.com/unitsofmeasurement/indriya/wiki/Unit-Equivalence

@keilw
Copy link
Member

keilw commented Jan 11, 2024

The use case of OpenHAB is mainly something like EUR/kWh, or maybe the cost per ton of CO2, etc., not converting Euros into Dollars or Radian.
In the meantime, what about adding new custom converters (which as mentioned above are needed for EUR/kWh or similar) to normalFormOrder in Calculus?
And how should its order look like, between the built-in ones and AbstractConverter.Pair or at the end of the list?

@jpink
Copy link
Author

jpink commented Jan 17, 2024

@andi-huber: I agree that there will be challenges to convert many different kinds of dimensionless units. But I think this library wasn't birth because of ideological or theoretical reasons. It should solve real world problems and there is no need to try convert radians to USD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 📋 Backlog
Development

No branches or pull requests

5 participants