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

unable to use custom unit_registry nor modify base_value for unit in default_unit_registry #473

Closed
AdamCohan opened this issue Nov 6, 2023 · 15 comments · Fixed by #475 or #476
Closed
Labels
bug Something isn't working

Comments

@AdamCohan
Copy link

  • unyt version: 2.9.5
  • Python version: 3.10.1
  • Operating System: macOS Ventura 13.2

Description

I have two issues. The first is when creating a custom unit_registry I can't seem to be able to be able to push the changes in any meaningful way to use the units in said registry. The second is when adding custom units to the default unit registry, I can't modify the base_value after the fact.

What I Did

import unyt

# ISSUE 1
my_registry = unyt.UnitRegistry()
unyt.unit_object.define_unit('bushel', 0.5*unyt.kg, registry=my_registry)
a = 2. * unyt.bushel

Traceback (most recent call last):
  File "/Users/adamcohan/Desktop/Clinic/pygreet/testing_units/testing_registries.py", line 5, in <module>
    a = 2. * unyt.bushel
AttributeError: module 'unyt' has no attribute 'bushel'

# ISSUE 2
unyt.unit_object.define_unit('bushel', 0.5*unyt.kg)
print(unyt.bushel.base_value)
print(unyt.unit_registry.default_unit_registry['bushel'])
unyt.unit_registry.default_unit_registry.modify('bushel', 0.1*unyt.kg)
print(unyt.bushel.base_value)
print(unyt.unit_registry.default_unit_registry['bushel'])

0.5
(0.5, (mass), 0.0, '\\rm{bushel}', False)
0.5
(0.1, (mass), 0.0, '\\rm{bushel}', False)
@AdamCohan
Copy link
Author

And yes, for issue 1 I've tried replacing unyt.bushel with my_registry.bushel, my_registry['bushel'], and unyt.Unit(my_registry['bushel']) and none of those seem to have worked either.

@neutrinoceros
Copy link
Member

Hi @AdamCohan

Issue 1

The bug is in your code sample itself, not unyt: if you create a new unit via a custom instance of UnitRegistry, it won't show up in the unyt namespace, by design, because only units defined in the default, builtin registry, can be accessed this way.

And yes, for issue 1 I've tried replacing unyt.bushel with my_registry.bushel, my_registry['bushel'], and unyt.Unit(my_registry['bushel']) and none of those seem to have worked either.

This is how you do it:

from unyt import Unit
u = Unit("bushel", registry=my_registry)

See https://unyt.readthedocs.io/en/stable/usage.html#unit-registries

Issue 2

The second is when adding custom units to the default unit registry, I can't modify the base_value after the fact.

I'm not 100% sure whether this is a bug or a feature actually. Modifying the base value of a unit in the default registry seems risky, and it's not something that we seem to support (there are no tests for it at the moment).
What we do support however is modifying units from a custom registry, so please let me know if that works for you.

Whatever the intended behaviour is, I can see there's room for improvement in unyt since at the moment, it just silently refuses to follow your instructions; I think it should either be allowed or raise an explicit error. @jzuhone do you have opinions here ?

@jzuhone
Copy link
Contributor

jzuhone commented Nov 6, 2023

We shouldn't allow modifying the base_value in the default unit registry--it's too risky. We should raise an explicit error for this.

@neutrinoceros
Copy link
Member

@jzuhone I've opened #475 to implement this behaviour

@AdamCohan
Copy link
Author

This worked, thank you.

@AdamCohan
Copy link
Author

I am having having an issue similar to issue 2 where instead of using the default_unit_registry I am using a custom one but it still isn't being modified:

import unyt

my_registry = unyt.UnitRegistry()

unyt.unit_object.define_unit('su', 0.5 * unyt.m, registry=my_registry)
silly_unit = unyt.Unit('su', registry = my_registry)
quantity1 = 2. * silly_unit

my_registry.modify('su', 0.1)
silly_unit = unyt.Unit('su', registry = my_registry)
quantity2 = 2. * silly_unit

quantity1.convert_to_units(unyt.m)
quantity2.convert_to_units(unyt.m)
print(quantity1)
print(quantity2)

> 1.0 m
> 1.0 m # should be 0.2 m

@AdamCohan
Copy link
Author

To clarify, with additional print statements I can tell that the registry is being modified, but for whatever reason the Unit constructor isn't using the updated value

@AdamCohan
Copy link
Author

I think it's an issue with the registry not being reloaded, because if I run my_registry = unyt.UnitRegistry.from_json(my_registry.to_json()) after the modification line it works fine.

@neutrinoceros
Copy link
Member

interesting, then I think that's another bug we need to deal with. I'll try to give it a go.

@neutrinoceros neutrinoceros reopened this Nov 10, 2023
@neutrinoceros neutrinoceros added the bug Something isn't working label Nov 10, 2023
@neutrinoceros
Copy link
Member

neutrinoceros commented Nov 10, 2023

Sorry I shouldn't have jumped to conclusions. With a little more time to think about it I don't think this is actually a bug; if you modify the base value of a unit, you also change the value of any previously defined quantities attached to that unit, because Unit objects retrieved from a registry are singletons: there's only one of each, hence modifying its definition affects all its users. This may not match you expectations but I think this is the intended behaviour. Maybe @jzuhone can confirm or infirm my reasoning.

@neutrinoceros
Copy link
Member

Maybe we can help you with what you're trying to accomplish if you give us a little more details.

@AdamCohan
Copy link
Author

If I understand what you're saying correctly then I think both of the 1.0 m results should be 0.2 m. What I'm trying to do is model if I were to make a custom Unit incorrectly (fat finger the wrong digit or something) then going back to fix it in the registry. quantity1 represents a value made with the wrong unit base value. I then update modify the registry to have the correct base value and create quantity2. When converting both of them to m, they both use the incorrect base value that was defined initially, when I would expect quantity2 or maybe even both of them to use the updated base value. Hopefully I'm not thinking about this entirely wrong. Thanks.

@AdamCohan
Copy link
Author

If instead the second block looks like this:

my_registry.modify('su', 0.1)
my_registry = unyt.UnitRegistry.from_json(my_registry.to_json()) # NEW LINE
silly_unit = unyt.Unit('su', registry = my_registry)
quantity2 = 2. * silly_unit

then everything seems to work as I would expect it to, where quantity1 converts to 1.0 m (the incorrect, original base value) and quantity2 converts to 0.2 m (the correct, new base value).

@neutrinoceros
Copy link
Member

If I understand what you're saying correctly then I think both of the 1.0 m results should be 0.2 m.

you're right, that is what I'd expect indeed. Somehow I missed that, thanks for pointing it out.

Hopefully I'm not thinking about this entirely wrong.

You're not. Your reasoning makes sense to me and I agree now that the modify method seems broken. I'll dig further in.

@neutrinoceros
Copy link
Member

neutrinoceros commented Nov 11, 2023

I've identified a bug in caching as the source of the current behaviour. #476 addresses it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants