-
Notifications
You must be signed in to change notification settings - Fork 477
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
Implement sorting units by dimension order #1864
Conversation
Once upon a time, ISO 80000 attempted to codify preferences for ordering units when describing quantities, for example `kW h` (not `h Kw`). While they have withdrawn the standard, there are many publications that state a preference for ordering units when describing quantities. Pint allows users to choose to sort units alphabetically or not (thus `kW * h` becomes `h * kW`, whereas `kW * s` retmains `kW * s` becase `kW` sorts as less than `s`). This PR adds a `sort_dims` parameter to `pint.formatting.formatter` which can be used in conjunction with alphabetical sorting. `sort_dims` imposes a "most significant dimension" order on the units, which are then sorted alphebetically (or not) within each dimension type. This addresses hgrecco#1841. It is intended as a prototype to evaluate as pint's formatting roadmap evolves. In particular, it needs a way to make `sort_dims` more accessible to the user. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Forgot to run pre-commit for initial commit. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
CodSpeed Performance ReportMerging #1864 will not alter performanceComparing Summary
|
Could dim_order be part of the registry so it can be modified by the user? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this should be a separate option, or whether it would be better to change sort
to take a string instead of a boolean: sort="iso80000"
(or something similar).
I think in addition to a registry option, we might also need a unit modifier.
OK...I stuck dim_order into defaults. kVA and N•m both follow the defined dim_order (and fix the alphabetization problem), but inch•lbf is backward. I did a quick Google search and most references came up with lbf•inch (or lbf•ft), which agrees with dim order. Perhaps you have been reading too many Pint outputs 🤓. I'll expand the test case and push a new commit in a few. |
Per suggestions from @andrewgsavage make `sort_dims` default. Also allow users to specify dimensional sorting in registry definition. Also fix a number of sub-tests not previously tested. Tests all pass without any drama. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Incorporate more code review feedback and fix some doc issues. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Remove `breakpoint()` left in by mistake. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
More attempts to make doc text and doc build happy. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
See whether I've turned off doctest where old behavior is no longer easily reproducible. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
And another attempt. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
I really like this. A few things:
class GenericPlainRegistry
formatter: Formatter
def __init__(self, ......):
self.formatter = Formatter(<here goes the current formatting arguments>) The Then to change the behavior ureg.formatter.locale = "de_DE" What are the advantages?
(names are open for discussions) |
Thanks for the comments! The first few could easily be done in the next commit. Would you rather the format changes be part of this PR (generalizing it considerably--not a bad thing) or would you rather land the Formatter class changes first and then adapt these changes to that? I'm OK either way, I just don't want to head down the wrong path and need to backtrack. |
I think we can do both things in the same PR as long as we do not change the default behaviour. |
Ok. It's going to take me a hot minute to wrap my head around the delegated behaviors and translate |
@MichaelTiemannOSC Awesome. I am open to talk about it if you need/want. I did this for the parser (although is not exposed to the user) some releases ago, and it was great. Briefly, the parser was part of the registry, deeply integrated. This made it very hard to test/fix/improve. Even replace (there were some proposals to replace the definition file format). What I did was to create the delegates submodule and the parser is there. Now the registry use that parser which is a standalone thing that emits definitions. |
Thanks for the offer. I would be happy to read any pseudocode you want to drop in the comments! I found the secret decoder ring here: #1595 |
Am I correct in thinking that the If so, then similarly convert various formatters from stand-alone functions to formatting functions in a class hierarchy (PrettyFormatter, CompactFormatter, etc)? |
Indeed, but the default formatter should inherit from all of them (I think). |
What do you suggest is the bridge to |
The I think that UnitsContainer should not be formatted. We could, during the transition, create a Formatter object hanging from that module that is called by UnitsContainer until we can deprecate that behaviour. |
Thanks for that. It confirms that I asked the right question ;-) |
By the way, UnitsContainer really doesn't want to be formatted, because it gets in a quite thick circular dependency with the Formatter definition in |
Can you put your code online, maybe in your github and I would be happy to take a look. |
Thanks! I've got many things sorted, and of course things are a mess. Currently stuck on how to merge these concepts:
with the Formatter. Current status is
With 64 of the 179 problems coming from the factoring of facets/measurement/objects.py and Anyway, here comes the ugliness before things are working again... |
Here is the WIP branch (branched from dim_order): https://github.com/MichaelTiemannOSC/pint/tree/dim_order_wip |
I think I see the path forward: @hgrecco if you see a major flaw with that assumption, let me know. In the meantime, I'm going to proceed with that assumption. |
I've looked more deeply at how the SharedRegistryObject, how the Quantities and the Units come together, and the magical mechanics of calls to |
The way I see it is the following: class Formatter:
def format_unit(self, unit, format_spec):
# do something
def format_quantity(self, quantity, format_spec):
# do something
class Quantity:
def __format__(self, format_spec):
return self._REGISTRY.formatter.format_quantity(self, format_spec)
class Unit:
def __format__(self, format_spec):
return self._REGISTRY.formatter.format_unit(self, format_spec) |
Thanks for that. I may not be able to get to it until mid-November due to upcoming travel. |
@MichaelTiemannOSC I can try to create a skeleton in a different branch and then you flesh it out. |
Take a look at this branch: https://github.com/hgrecco/pint/tree/formatter_delegate |
I merged part of the formatter class into develop. As soon as I test it I will merge it into master |
Picking up latest changes from hgrecco and crew.
Prepare to merge `develop` into this PR. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Move the proposed `dim_sort` functionality from being a system registry property to being a formatter property, specifically `BaseFormatter`. Also fix typos noticed in `pint/testsuite/conftest.py`. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Based on this error message from the CI build: ``` Run sphinx-build -n -j auto -b html -d build/doctrees docs build/html sphinx-build -n -j auto -b html -d build/doctrees docs build/html shell: /usr/bin/bash -e {0} env: pythonLocation: /opt/hostedtoolcache/Python/3.9.18/x64 LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.18/x64/lib Running Sphinx v4.5.0 Sphinx version error: The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0; it therefore cannot be built with this version. Error: Process completed with exit code 2. ``` bump sphinx to >= 5 (>4 only got us to 4.5). Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
This reverts commit 41dcc42.
@hgrecco I know enough to know I don't know how to fix the sphinx problems. I'm sure there's some history embedded in the version choices, but I don't know it. Otherwise, all green. |
This looks great. I will finish creating the formatter and then proceed to incorporate all these. |
formatter changes. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Closing because obsoleted by #1926 |
Once upon a time, ISO 80000 attempted to codify preferences for ordering units when describing quantities, for example
kW h
(noth Kw
). While they have withdrawn the standard, there are many publications that state a preference for ordering units when describing quantities.Pint allows users to choose to sort units alphabetically or not (thus
kW * h
becomesh * kW
, whereaskW * s
retmainskW * s
becasekW
sorts as less thans
).This PR adds a
sort_dims
parameter topint.formatting.formatter
which can be used in conjunction with alphabetical sorting.sort_dims
imposes a "most significant dimension" order on the units, which are then sorted alphebetically (or not) within each dimension type.This addresses #1841. It is intended as a prototype to evaluate as pint's formatting roadmap evolves. In particular, it needs a way to make
sort_dims
more accessible to the user.pre-commit run --all-files
with no errors