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

What does registry=None mean in custom formatter ? #2009

Closed
aulemahal opened this issue Jun 10, 2024 · 5 comments · Fixed by #2011
Closed

What does registry=None mean in custom formatter ? #2009

aulemahal opened this issue Jun 10, 2024 · 5 comments · Fixed by #2011

Comments

@aulemahal
Copy link
Contributor

aulemahal commented Jun 10, 2024

Hi!

With pint 0.24, the internal workings of register_unit_format have changed and the registry argument of the wrapped function can now be "None". I do not see in the doc the meaning of that new possible value. Wouldn't a Unit object always be related to a registry, even if it's the default one ?

This is causing issues in cf-xarray (and downstream) for example (xarray-contrib/cf-xarray#507) where we used registry to convert the UnitContainer back to a Unit (see https://github.com/xarray-contrib/cf-xarray/blob/24dd81b2748db59c3ede68735a200b63b8a3d2d2/cf_xarray/units.py#L17-L38)

EDIT: Also, the way register_unit_format is written, wouldn't registry always be None ?

@hgrecco
Copy link
Owner

hgrecco commented Jun 10, 2024

First, sorry for breaking things. The move to the new Formatter delegate allows more flexibility so I think this annoyance will pay off. Let me answer each of your questions:

Wouldn't a Unit object always be related to a registry, even if it's the default one ?
The formatter also format unit like objects (i.e. dicts mapping names to exponents) which do not necesesary require a registry.

Also, the way register_unit_format is written, wouldn't registry always be None
Yes. I was planning to try a solution but there was no need until now.

The idea would be to make this function look and instantiate the registered formatters.

    def get_formatter(self, spec: str):
        if spec == "":
            return self._formatters["D"]
        for k, v in self._formatters.items():
            if k in spec:
                return v

        try:
            orphan_fmt = REGISTERED_FORMATTERS[spec]
        except KeyError:
            return self._formatters["D"]

        try:
            fmt = orphan_fmt.__class__(self._registry)
            spec = get_attr(fmt, "spec", spec)
            self._formatters[spec] = fmt
            return fmt
        except:
            return orphan_fmt

@hgrecco
Copy link
Owner

hgrecco commented Jun 10, 2024

and then also add the spec attribute to NewFormatter here

@hgrecco
Copy link
Owner

hgrecco commented Jun 10, 2024

On a second thought
spec = get_attr(fmt, "spec", spec) and adding the spec attribut might not be necessary.

@aulemahal
Copy link
Contributor Author

aulemahal commented Jun 10, 2024

I think I understand, thanks! I think I can try to suggest these changes in a PR if that helps.

@hgrecco
Copy link
Owner

hgrecco commented Jun 10, 2024

That would be great. I have review the code, it is necessary to add spec = get_attr(fmt, "spec", spec) and adding the spec attribute to NewFormatter. Let me know if you need help.

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 a pull request may close this issue.

2 participants