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

BUG: fix caching error with UnitRegistry.modify #476

Merged
merged 4 commits into from
Jan 28, 2024

Conversation

neutrinoceros
Copy link
Member

close #473

@neutrinoceros neutrinoceros force-pushed the hotfix_registry_cache_error branch 2 times, most recently from 5bf52c5 to 99cf350 Compare November 11, 2023 13:45
@neutrinoceros neutrinoceros added the bug Something isn't working label Nov 11, 2023
@neutrinoceros neutrinoceros force-pushed the hotfix_registry_cache_error branch from 2aee0fc to d8b8e91 Compare November 11, 2023 13:50
u2 = Unit("celery", registry=ureg)
assert 1.0 * u2 == 0.5 * u0

# TODO: test behaviour in u1 pre/post modification
Copy link
Member Author

Choose a reason for hiding this comment

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

this line refers to the Unit created on after u0 (I guess linters removed the variable binding to u1 because I didn't use it). The behaviour I'm getting with the branch at the moment is that the Unit object created before ureg.modify still uses the original base value even after modification of the registry. This seems to be in line with @AdamCohan's expectations, and seems consistent with the idea that Unit objects should be immutable, but I don't think that this case is currently tested. @jzuhone, what should be the expected behaviour in your opinion ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's try that again: @jzuhone

Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior you are seeing is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you ! I've updated the test to reflect this and rebased the branch, this is now ready for review !

@neutrinoceros neutrinoceros force-pushed the hotfix_registry_cache_error branch from 24c7864 to 0f62901 Compare January 28, 2024 08:38
@neutrinoceros neutrinoceros marked this pull request as ready for review January 28, 2024 08:39
@jzuhone jzuhone merged commit cb71e02 into yt-project:main Jan 28, 2024
9 checks passed
@neutrinoceros neutrinoceros deleted the hotfix_registry_cache_error branch January 28, 2024 18:23
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
Development

Successfully merging this pull request may close these issues.

unable to use custom unit_registry nor modify base_value for unit in default_unit_registry
2 participants