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

Adds a currency dimension #273

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

samgdotson
Copy link
Contributor

This PR introduces a currency dimension with base units of $ and ¢. Now, a quantity with currency units can be treated like any other quantity. Previously there was no way to handle a unit such as $/MWh without treating it as a quantity with dimensions 1 / energy = 1 / (force x length).

Although this is the same as other units, I include the basic usage.

from unyt import unyt_quantity
from unyt import dollars, cents, MW, kW

USD = 1*dollars
print(USD)
# unyt_quantity(1., '$')
print(USD.to(cents))
# unyt_quantity(100., '¢')

capital_cost  = "1000 $/MW"
capital_cost_unyt = unyt_quantity.from_string(capital_cost)
print(capital_cost_unyt)
# unyt_quantity(1000., '$/MW')

capacity = 300*kW
print(capital_cost_unyt*capacity)
# unyt_quantity(300., '$')

Comments

Currency is a useful and common type of quantity (especially in economic fields). This functionality has been sought previously (#178 ). However, it's not a physical unit, unlike time or length. I'm a little uncomfortable including currency as a base unit in a unit system (especially one like the galactic_unit_system). Yet, the testing suite expected some lines in usage.rst to indicate that currency is an attribute. I'm not sure of a solution to this, or if it's even really a problem. I'm happy to make changes. Thank you for taking the time to review this!

@neutrinoceros
Copy link
Member

First of all, thanks for your work. If I'm being honest I am not sure how I feel about it because, contrary to physical units that unyt focuses on, conversion factors between currency units are not constants. So it seems to me that the way you implemented it, we cannot ever extend builtin definitions to include other currencies than the dollar, and I'm not very confortable with making unyt less relevant to users outside of the US.

This functionality has been sought previously (#178 )

The way I read it, that ticket was more about the problem of having a new API to allow definitions of custom dimensions in user code. For reference I would be 100% on board with that, but that's a very different approach.

As it stands, I'd be tempted to say this is out of scope, though it's not my call, and I'll defer to @jzuhone.

@samgdotson
Copy link
Contributor Author

Thanks for looking over my PR.

contrary to physical units that unyt focuses on, conversion factors between currency units are not constants.

This is true, and I completely agree that a currency conversion functionality is outside the scope of unyt. There are other packages that focus on converting currency. However, the most common use case for a currency unit is working within a single currency. Given the universality of money, it makes sense that unyt should be able to handle units-of-money. Making the default currency dollars was a biased, but ultimately arbitrary, choice.

So it seems to me that the way you implemented it, we cannot ever extend builtin definitions to include other currencies than the dollar.

I simply don't see how this is true. Adding the dollar definition was straightforward. The primary contribution of this PR is a currency dimension. Also, a key feature of unyt is the ability to create your own units with a unit registry. I can make a euro unit as easily as I could make fortnights or other unit with a unit registry. See the code snippet below

In [1]: from unyt import UnitRegistry, Unit

In [2]: from unyt.dimensions import currency

In [3]: reg = UnitRegistry()

In [4]: reg.add('EUR', base_value=1.0, dimensions=currency, tex_repr="\texteuro")

In [5]: 'EUR' in reg
Out[5]: True

In [6]: euro = Unit('EUR', registry=reg)

In [7]: data = 3*euro

In [8]: print(data)
3 EUR

In [9]: print(data.units.dimensions)
(currency)

Lastly, the way I understood ticket #178 was that
A) the user wanted to use a currency unit
B) saw that unyt had no builtin currency unit, so asked how to make one
C) discovered that user-defined dimensions were not possible and moved on.

I had an identical thought process, but I made a PR instead of moving on :). Not having a way to handle currency means losing out on potential users.

All that said, I'm happy to make any requested changes such as:

  • adding more currency types (euro, yen, pound, rupee, AUSD, CAD)
  • adding more documentation to outline the limitations of the currency units
  • changing the default currency unit in the existing unit systems

Again, thank you for taking the time to review this.

@neutrinoceros
Copy link
Member

I simply don't see how this is true. Adding the dollar definition was straightforward. The primary contribution of this PR is a currency dimension. Also, a key feature of unyt is the ability to create your own units with a unit registry. I can make a euro unit as easily as I could make fortnights or other unit with a unit registry.

To be clear, what I meant is that we cannot add e.g. euros (with a fixed conversion factor to dollars) to unyt itself. I reckon that it's still doable from the user standpoint.

I had an identical thought process, but I made a PR instead of moving on :).

And that's absolutely fine, awesome, even !

Not having a way to handle currency means losing out on potential users.

That's also true, I just replied from a maintainer standpoint. Maintaining software is also about making choices of what we want to support, because we have limited bandwidth and workforce to make sure the code continues to work in the coming years. Again, this is not my call, ultimately.

@jzuhone
Copy link
Contributor

jzuhone commented Sep 21, 2022

So one of the fundamental functions of unyt is to convert between various units of the same dimension. And I share the concerns of @neutrinoceros that something seems to be fundamentally missing if we cannot convert between US currencies and at least a few other common currencies.

I would actually be happier with this idea if we found a way to optionally hook into some other python library that handled the exchange rates, but of course that introduces more complexity and a dependence on an internet connection.

Let me sit on this for a day or two--not sure about it.

@matthewturk @chrishavlin Want to chime in?

@chrishavlin
Copy link
Contributor

Chiming in after a day of mulling it over...

Ideally, I'd prefer a refactor to allow user-defined dimensions... but without that, I think having a base currency dimension is reasonable.

I do not think we should stray into currency conversion. If unyt had a currency dimension, it'd be straightforward for someone who's interested to write their own package utilizing unyt and other tools (e.g., a unyt_currency_converter), but building in dependencies to dynamic exchange rate packages seems well out of scope for base unyt. One extra wrinkle to currency conversion that I noticed while looking into the idea -- currency conversion may not be reversible! Depending on how exchange rates are calculated, USD/EURO will not always equal 1 / (USD/EURO). That would be impossible to handle with unyt, and if someone was trying to handle that I'd have to tell them to use pint (which can handle that).

So IMO, it's a question of how much value there is in adding a new base dimension with one unit associated with it. If I were using unyt for currency, I'd be fine defining a set of nondimensional units relative to each other:

import unyt 

unyt.define_unit('EUR', unyt.unyt_quantity(1,''), tex_repr="\texteuro")
euros_per_dollar = 1.2
unyt.define_unit('USD', unyt.unyt_quantity(euros_per_dollar,'EUR'), tex_repr="\$")
unyt.define_unit('kWh', unyt.unyt_quantity(1, unyt.kW * unyt.hr ), tex_repr="kWh")

electric_rate = unyt.unyt_quantity(0.15, 'USD/kWh')
kWh_used = unyt.unyt_quantity(100., 'kWh')
cost = electric_rate*kWh_used
print(cost.to("EUR"))
# 18.0 EUR

the only downside to that is that you lose the dimensionality validation, and you could add any nondimensional quantity to these "currency" units. But since currency doesn't ultimately have any fixed physical constants to define it, IMO it makes some sense to treat it as nondimensional...

So if @samgdotson was setting off to start the PR I probably would have said use nondimensional quantities or just use pint :) but since the PR is here, why not include it? I would prefer a more general unit than "dollar" though. Perhaps replace "dollar" with "monetary_unit" and require that users define their own units if they want to work in USD, ERUO, etc? You could still add the changes to _parsing.py to handle currency symbols, though I would request we add more than USD.

@samgdotson
Copy link
Contributor Author

Thanks, @chrishavlin. Treating monetary units as dimensionless enables most of the functionality with unyt, except for a few important features (IMO):

  1. dimension validation (as you mentioned)
  2. cannot use monetary codes as unit repr because of sympy
import unyt
unyt.define_unit('$', unyt.unyt_quantity(1.0, ''), tex_repr="\$")

UnitParseError: Unit expression '$' raised an error during parsing:
SyntaxError('unexpected EOF while parsing', ('<string>', 1, 1, '$'))
  1. cannot use the convenient from_string functionality (because of item 2).

I'm happy to add additional currencies to the PR to improve relevance.

@samgdotson
Copy link
Contributor Author

Just to provide some more context for this feature, I am developing a set of packages for energy system planning and analysis. Enabling dimension validation with currencies and parsing from a string would make unyt a good dependency for my work. @chrishavlin's example is definitely one important application. Additionally, some datasets contain units in string format, which I would like to read in with unyt. For example, I might have data like

value unit
12.00 $/MWh
00.23 $/kWh

that would be useful to convert with unyt rather than writing my own string parser. There aren't any energy system packages that have integrated unit handling, requiring users to convert units themselves, which is an error-prone practice. Somewhat separately, I prefer unyt to pint because it has less overhead for users since it doesn't require users to instantiate a unit registry.

Again, let me know if there is anything else I can add to this PR to make it a good addition to unyt.

@ngoldbaum
Copy link
Member

ngoldbaum commented Oct 2, 2022

I agree with others that implementing conversions between currencies is out of scope, but I also think that the original need is well-motivated and we should support dealing with currencies. The examples in the PR description, where currency mixes with physical units, is common enough in engineering that we should have a way to deal with it with as little friction as possible.

I also think adding an API to add custom dimensions is probably useful too (although are there any other motivating needs we've seen come up besides currency?).

I just checked, and in this PR you can do things like this, which is definitely not right:

 In [1]: import unyt

In [2]: unyt.__version__
Out[2]: 'v2.9.2+91.gc9881e4'

In [3]: (1*unyt.dollar).to("yen")
Out[3]: unyt_quantity(1., '¥')

The problem is that all the currency units you've added have the same base_value:

In [4]: unyt.dollar.base_value
Out[4]: 1.0

In [5]: unyt.yen.base_value
Out[5]: 1.0

I think a compromise between everyone's concerns about variable exchange rates and the stated desire from users to deal with currencies is to not have any currencies defined out of the box, but to have an API like:

unyt.add_currency("dollar", 1.0, "$", aliases="USD")
unyt.add_currency("yen", .0069, "¥", aliases=["JPY", "JP¥"])

You could also have an API that adds multiple currencies in the same line, but I think just allowing one at a time makes the implementation have fewer corner cases to deal with.

To simplify this you could probably have a list of "known" currencies built into unyt and not require the aliases and symbols unless it's not in the list of known currencies. The value in the base currency unit probably needs to be a positional argument since we can't know that ahead of time without adding full-blow "real" currency conversion to unyt.

Note that this is just me spit-balling the API with an example, if you want to make it more or less flexible, please feel free, just be sure to add docs and tests. Think carefully about default behavior for any keyword arguments, as well as error handling for invalid inputs.

Note that this doesn't deal with the issue @chrishavlin noted, where vagaries of finance may mean that round-tripping conversions might yield fiscally incorrect results. One way to handle that would be to just note it in the docs and say that unyt's currency handling assumes currency conversions can round-trip, which, along with assuming a fixed currency conversion is probably more than enough for most engineering calculations that one would want to use unyt for.

#: cost per energy
cost_per_energy = cost_per_erg = currency / energy
#: cost per power
cost_per_power = currency / power
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these derived dimensions need to be added with explicit aliases in this namespace unless they're going to be used elsewhere inside unyt.

cgs_unit_system = UnitSystem("cgs", "cm", "g", "s", current_mks_unit=None)
cgs_unit_system = UnitSystem(
"cgs", "cm", "g", "s", currency_unit="$", current_mks_unit=None
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since currency_unit defaults to "$" you don't need to edit the definitions of any of the built-in unit systems you've edited here or below

@ngoldbaum
Copy link
Member

Another option would be to not allow conversions between currencies at all and only allow one currency at a time

@samgdotson
Copy link
Contributor Author

Thanks @ngoldbaum! Personally, I'm leaning towards blocking currency conversion entirely because it's easier to implement and maintain and is more robust. With respect to engineering calculations, it's much more common to use a single currency than to convert between currencies (except between something like cents and dollars).

I can also add examples for using currency conversion from another library + unyt to the docs if that would be helpful.

@samgdotson
Copy link
Contributor Author

@ngoldbaum this PR now includes

  • tests for currency creation and conversion
  • blocks currency conversion between currencies (allows dollars -> cents and vice versa)

@ngoldbaum
Copy link
Member

I'll try to look over this soon. However it still needs documentation before it can be merged.

@samgdotson
Copy link
Contributor Author

It's been a few weeks, just checking on the status of this pull request. Thanks!

@ngoldbaum
Copy link
Member

Sorry, I thought you weren't done because you didn't implement what I suggested when we talked about this previously. I still think having pre-defined currencies "out of the box" is a bad idea because it will inevitably ignore most world currencies. Having support out of the box to make the experience nicer for common currencies is fine though. I also still think a unyt.add_currency function would be the best way to handle opting in to currency support and allowing people to work in the currency(ies) they'd like to work in.

@samgdotson
Copy link
Contributor Author

I see that I misunderstood your initial comments. The solution that I implemented was that users are simply not allowed to convert among units with dimensions of currency (with tests and docs).

Another option would be to not allow conversions between currencies at all and only allow one currency at a time

However, now it seems like that isn't what you wanted. I am confused about your suggestion to implement an add_currency function because I'm not sure how it differs from the existing API to define_unit or create a unit with a UnitRegistry -- both of which can be used to create arbitrary currencies with a currency dimension -- except without automatic aliasing (aliases could be added, if only individually using define_unit). For example:

In [1]: import unyt as u
In [2]: peso = 0.017*u.dollars
In [3]: u.define_unit("peso", peso, tex_repr="\textpeso")
In [4]: 1*u.peso
Out[4]: unyt_quantity(1, 'peso')

Although, defining the unit in terms of dollars doesn't mean anything right now because all currency conversions are currently blocked. I could restore the ability to convert among currencies with unyt, but then it seems we return to the issue of being able to convert $1 to 1 yen (peso, euro, won, etc).

That being said, I have two further comments:

  1. There are some significant problems parsing unicode symbols, such as ₱, to the point where it seems impossible to define a unit using these symbols -- which requires their definition out of the box. In the example above, I cannot define a peso with its unicode symbol --
In [3]: u.define_unit("\u20b1", peso, tex_repr="\textpeso")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
ValueError: Error from parse_expr with transformed code: '₱'
...
UnitParseError: Unit expression '₱' raised an error during parsing:
SyntaxError("invalid character '₱' (U+20B1)", ('<string>', 1, 1, '₱'))

Which is solved by adding unit_expr = unit_expr.replace("\u20b1", "peso")
in _parsing.py, but leads to the following problem error.

In [3]: u.define_unit("\u20b1", peso, tex_repr="\textpeso")
---------------------------------------------------------------------------
UnitParseError                            Traceback (most recent call last)
Input In [3], in <cell line: 1>()
----> 1 u.define_unit("\u20b1", peso, tex_repr="\textpeso")
...
UnitParseError: Could not find unit symbol 'peso' in the provided symbols.

Which I have only been able to rectify by explicitly adding a peso unit in the default LUT.

  1. I think users should be able to use common currencies out of the box for the same reason that unyt doesn't require users to create a unit registry (i.e. limiting cognitive overhead).

TL;DR

I think creating an add_currency function duplicates some existing functionality and requiring this pattern in order to use the currency dimension seems antithetical to the ease-of-use that makes unyt appealing.

Would a zoom call be helpful for clarity?

@ngoldbaum
Copy link
Member

I'd like to have an API for users to add their own currencies without modifying unyt. You're right that in principle someone could use define_unit to do that, but I'd also like the currencies to be opt-in, e.g. the currency dimension doesn't get used by unyt until someone calls add_currency.

For the parsing issue, that's resolved by the API I originally suggested, you'd do:

unyt.add_currency("yen", .0069, "¥", aliases=["JPY", "JP¥"])

This would make it so you can do unyt.yen, and you'd get back a unyt.Unit object with a text repr of ¥ and dimensions of currency. You're right that someone couldn't do unyt.¥, that's a limitation of python variable names and it's ok that we don't support that.

I also think that making currencies not convertible between each other is orthogonal from the API I'm proposing and doesn't preclude adding that API.

@samgdotson
Copy link
Contributor Author

Okay, I can definitely include the add_currency feature you're requesting. For clarification, I understood a "list of known currencies" to mean some set of pre-defined common currencies (e.g. dollars, euros).

My desired behavior would be

import unyt as u

# import known currencies
from unyt import dollars, euros

# add a new currency
u.add_currency("yen", .0069, "¥", aliases=["JPY", "JP¥"])

Is this what you are envisioning? I think it's important that users are not required to call a function before using a common currency -- just like users aren't required to create a registry before using any other unit.

@ngoldbaum
Copy link
Member

I don't think we should have currencies enabled out of the box. I think it should work like this for common currencies:

import unyt
unyt.add_currency("dollar")

# doesn't raise an AttributeError
unyt.dollar

This is because we don't want common currencies to be specially privileged compared to other currencies. Please see the replies you got in this thread initially for opinions from other unyt maintainers that agree with this.

I'm sorry we're having such trouble coming to agreement on this.

@ngoldbaum
Copy link
Member

Also, as I said earlier, it's fine if we make the call to add_currency "simplified" for common currencies, I just want users to manually opt-in to enabling currencies.

Also if you decide to make currencies not convertible out of the box, maybe we should only allow one currency to be defined at a time?

@neutrinoceros
Copy link
Member

maybe we should only allow one currency to be defined at a time?

Unless there's an obvious use case this would prevent, I'd be in favour of this: it's always simpler to add things later than removing them.

@jzuhone
Copy link
Contributor

jzuhone commented Nov 3, 2022

I realize that this suggestion was made earlier and was shot down, but I'll say again I'd be open to optionally converting currencies if one had a package installed that managed exchange rates. I know this is an extra layer of complexity, and definitely not for this PR, but I think it's a neat enough idea to consider later.

@samgdotson
Copy link
Contributor Author

@ngoldbaum thank you for continuing to engage with this PR. I think having an API such as the add_currency you describe is good for handling the unique challenges of currency units (in particular their many possible aliases). I also appreciate not wanting to prioritize some currencies over others.

One important feature of unyt is the ability to parse data from strings. In order to facilitate this, I manually added symbols for several common currencies to a regex pattern. The add_currency API would not automatically enable this for a novel currency (for example, the won) because that symbol isn't in the regex. Even requiring users to opt-in to the currency dimension would not expose all of unyt's functionality, thereby privileging some currencies over others. I could add as many currencies to this regex as you would like, but then I might as well just define them in the default lookup table.

Do you have any ideas of a workaround to make it possible that the currency units can be parsed by the regex in unyt without having out of the box currencies?

@ngoldbaum
Copy link
Member

Maybe add_currency could manipulate the regex?

@neutrinoceros
Copy link
Member

+1
I further note that by "manipulate", I think Nathan means "copy and extend" rather than "change globally" (correct me if I'm wrong)

@sophiee-b
Copy link

Is this still being worked on? Is there any idea of a timeline for currencies being implemented in unyt?

@jzuhone
Copy link
Contributor

jzuhone commented Nov 11, 2023

@sophiee-b We don't have a timeline for this, as we couldn't agree on a few issues related to how it would work, given that currencies are changing value.

I'm not opposed to the idea myself, and I believe it could be done as an optional feature with support from a Python library that handles currencies.

If you'd like to help dig in with this, that would be most welcome.

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

Successfully merging this pull request may close these issues.

6 participants