-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: main
Are you sure you want to change the base?
Conversation
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.
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. |
Thanks for looking over my PR.
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.
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 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 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:
Again, thank you for taking the time to review this. |
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.
And that's absolutely fine, awesome, even !
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. |
So one of the fundamental functions of 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? |
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 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 |
Thanks, @chrishavlin. Treating monetary units as dimensionless enables most of the functionality with unyt, except for a few important features (IMO):
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, '$'))
I'm happy to add additional currencies to the PR to improve relevance. |
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
that would be useful to convert with Again, let me know if there is anything else I can add to this PR to make it a good addition to unyt. |
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:
The problem is that all the currency units you've added have the same
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:
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 |
#: cost per energy | ||
cost_per_energy = cost_per_erg = currency / energy | ||
#: cost per power | ||
cost_per_power = currency / power |
There was a problem hiding this comment.
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
.
unyt/unit_systems.py
Outdated
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 | ||
) |
There was a problem hiding this comment.
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
Another option would be to not allow conversions between currencies at all and only allow one currency at a time |
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. |
@ngoldbaum this PR now includes
|
I'll try to look over this soon. However it still needs documentation before it can be merged. |
…currency-dimension
…currency-dimension
It's been a few weeks, just checking on the status of this pull request. Thanks! |
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 |
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).
However, now it seems like that isn't what you wanted. I am confused about your suggestion to implement an 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 That being said, I have two further comments:
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 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.
TL;DRI think creating an Would a zoom call be helpful for clarity? |
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 For the parsing issue, that's resolved by the API I originally suggested, you'd do:
This would make it so you can do 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. |
Okay, I can definitely include the 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. |
I don't think we should have currencies enabled out of the box. I think it should work like this for common currencies:
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. |
Also, as I said earlier, it's fine if we make the call to 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? |
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. |
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. |
@ngoldbaum thank you for continuing to engage with this PR. I think having an API such as the One important feature of 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? |
Maybe |
+1 |
Is this still being worked on? Is there any idea of a timeline for currencies being implemented in unyt? |
@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. |
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 dimensions1 / energy = 1 / (force x length)
.Although this is the same as other units, I include the basic usage.
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 inusage.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!