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

Remove global state variables #89

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

pm-blanco
Copy link
Collaborator

@pm-blanco pm-blanco commented Sep 1, 2024

Solves #60

  • Removes global state variables, instead they are now created by the constructor of pyMBE.pymbe_library.
  • pmb.set_reduced_units() now redefines the reduced units instead of creating a new instance of pint.UnitRegistry. Therefore, the user can do operations between objects defining before and after changing the set of reduced units without getting a ValueError
  • pmb.set_reduced_units() now checks that the arguments provided by the user have the right dimensionality.
  • The constants stored as attributes in pyMBE.pymbe_library are not rewritten anymore after calling pmb.set_reduced_units()

@pm-blanco pm-blanco added bug Something isn't working code quality labels Sep 1, 2024
@pm-blanco pm-blanco added this to the pyMBE 1.0.0 milestone Sep 1, 2024
@pm-blanco pm-blanco self-assigned this Sep 1, 2024
@pm-blanco
Copy link
Collaborator Author

pm-blanco commented Sep 2, 2024

I believe this solves most of the concerns in #60. To me, the only downside of this solution is that it allows operations between quantities defined in different systems of reduced units:

import pyMBE

## The unit of length defaults to 0.335 nm here
pmb1 = pyMBE.pymbe_library(seed=42)

## Define a variables in the old unit registry
reduced_length_1=pmb1.units.Quantity(1,"reduced_length")

## Change the system of reduced units
pmb1.set_reduced_units(unit_length=0.4*pmb1.units.nm)

## Define variable in the new unit registry
reduced_length_2=pmb1.units.Quantity(2,"reduced_length")

## Ideally this should raise a ValueError because the two variables
## were defined using different definitions of reduced length

sum = reduced_length_1+reduced_length_2
print (sum)

>>> 3 reduced_length

I am not sure if we can do too much to prevent users from doing this though. This could happen also if we change the logic and let the user pass the UnitRegistry object as an input argument of the constructor of pyMBE and pmb.set_reduced_units

@jngrad
Copy link
Member

jngrad commented Sep 4, 2024

The proposed changes should fix most of the issues we have with the current code. Thanks!

I'm still convinced that letting users provide their own unit registry as argument and keeping it as a data member is the most elegant solution, because it would prevent us from mixing units from different registries. But then we would have to prevent users from calling set_reduced_units(), since as you correctly pointed out, it would be another way for them to accidentally mix units from different registries. Not great, unless we can detect if two units belong to different registries, and throw an exception. We would also have a situation where users are able to create new units from both pmb.units and from their own units registry (which are shared variables and have the same id), which can be confusing since modifying units will have the same effect as modifying pmb.units.

I can't really think of a better solution right now, and this PR is already a huge step in the right direction. I would consider #60 as closed, unless someone can come up with a use case where mixing units leads to unexpected results despite our use of units._build_cache().

@pm-blanco pm-blanco merged commit 587d025 into pyMBE-dev:main Sep 4, 2024
3 checks passed
@pm-blanco pm-blanco deleted the remove_global_variables branch September 5, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants