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

add a module-global registry #32

Merged
merged 22 commits into from Oct 26, 2020
Merged

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Sep 2, 2020

This is an attempt to fix the registry creation as discussed in #7. It will make sure that the selected registry is usable (i.e force_ndarray_like or force_ndarray are set) and that only a single registry is used. To make ds.pint.quantify() work without any boilerplate code, it also introduces a module-global unit registry (pint_xarray.unit_registry). For now, if someone wants a different unit registry, they need to call .quantify with that registry – subsequent calls to .quantify on the same object will then use that registry and reject other registries – or maybe set

pint_xarray.unit_registry = pint.UnitRegistry(..., force_ndarray_like=True)

I'm not quite sure if this is a good API so this is a draft for now.

@keewis keewis marked this pull request as ready for review September 4, 2020 15:58
@keewis
Copy link
Collaborator Author

keewis commented Sep 4, 2020

I'm still not sure if this API is a good choice, but i guess for now it's ready for reviews.

@keewis keewis mentioned this pull request Sep 4, 2020
7 tasks
@keewis
Copy link
Collaborator Author

keewis commented Oct 7, 2020

After thinking about this some more I realized that the changes in this PR might not be optimal: it introduces its own "application registry", but if every library chose that approach they wouldn't be able to work together (see also Unidata/MetPy#1350)

So I guess we need to either:

  • change or overwrite the global application registry on import
  • change or overwrite using a setup_registry function
  • raise on incompatible settings

where changing the registry would require a get_application_registry / set_application_registry cycle to avoid accessing internal parts of pint (I'd like to be able to use pint.application_registry instead, but module-level descriptors are currently only possible using module __getattr__ and a wrapper class, which is way too complicated).

If I'm honest, I'd prefer option 1 (change on import) since that would avoid boilerplate code but it might also lead to incompatible registries. The next best option might be a combination of 2 and 3?

Not sure if it would help, but maybe we can have some sort of set_application_registry_options function in pint that makes sure each option can only be set once? That way using incompatible libraries wouldn't fail silently.

cc @jthielen, @dopplershift

@keewis
Copy link
Collaborator Author

keewis commented Oct 22, 2020

with the recent changes, the default registry is the application registry, which is changed on import (option 1a and 2b, I guess?).

Even if this turns out to be undesirable it shouldn't be too difficult to change.

@keewis
Copy link
Collaborator Author

keewis commented Oct 22, 2020

This still doesn't solve the issue of multiple libraries requesting conflicting settings, but maybe we should wait until someone runs into this? If so I guess I'm content with the way it is right now, and I'll merge in a few days.

It would be good to have at least one review, though. @dcherian, maybe?

@keewis
Copy link
Collaborator Author

keewis commented Oct 26, 2020

alright, I'm merging. As I said before, pint-xarray is still experimental, so changing this if it turns out to be a bad decision shouldn't be difficult.

@keewis keewis merged commit fd0df21 into xarray-contrib:master Oct 26, 2020
@keewis keewis deleted the global-registry branch October 26, 2020 16:28
@jthielen
Copy link
Collaborator

@keewis Thanks for taking the lead on this and I agree with your assessment. Also, my apologies for my continued inattentiveness on reviews here, the past couple months have been particularly difficult to keep up with things. I hope to get back to helping out more on these things in early December after the fall semester wraps up. Until then, don't worry about me in pushing forward with anything here!

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.

Create global reference to default registry on import?
2 participants