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

Dim sort #1926

Merged
merged 7 commits into from
Mar 8, 2024
Merged

Dim sort #1926

merged 7 commits into from
Mar 8, 2024

Conversation

MichaelTiemannOSC
Copy link
Collaborator

Adapt PR#1841 to the new Pint formatter.

TODO: show we can override default in registry init file.

Adapt PR#1841 to the new Pint formatter.

Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
Copy link

codspeed-hq bot commented Jan 23, 2024

CodSpeed Performance Report

Merging #1926 will not alter performance

Comparing dim-sort (32f543d) with master (3cc2d36)

Summary

✅ 439 untouched benchmarks

Remove `breakpoint`s that should have been linted out by pre-commit.

Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
It would be easier to just default to `sorted` instead of `None`, but since `None` is an option, we have to test for it anyway.

Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
@MichaelTiemannOSC
Copy link
Collaborator Author

This test creates a UnitsContainer with meter inserted before kilogram:

    def test_unit_formatting(self, subtests):
        x = self.U_(UnitsContainer(meter=2, kilogram=1, second=-1))
        for spec, result in (
            ("{}", str(x)),
            ("{!s}", str(x)),
            ("{!r}", repr(x)),
	    (
                "{:L}",
                r"\frac{\mathrm{kilogram} \cdot \mathrm{meter}^{2}}{\mathrm{second}}",
            ),
            ("{:P}", "kilogram·meter²/second"),
            ("{:H}", "kilogram meter<sup>2</sup>/second"),
            ("{:C}", "kilogram*meter**2/second"),
            ("{:Lx}", r"\si[]{\kilo\gram\meter\squared\per\second}"),
            ("{:~}", "kg * m ** 2 / s"),
            ("{:L~}", r"\frac{\mathrm{kg} \cdot \mathrm{m}^{2}}{\mathrm{s}}"),
            ("{:P~}", "kg·m²/s"),
	    ("{:H~}", "kg m<sup>2</sup>/s"),
            ("{:C~}", "kg*m**2/s"),
        ):
            with subtests.test(spec):
                assert spec.format(x) == result

The UnitsContainer formatter, by default, sorts these alphabetically, putting kilogram before meter**2. But if the dimensional sort function is None, it imposes no sort, leaving insertion order to rule. Which leaves meter**2 before kilogram.

This argues for the idea that we must default sorting compound units with sorted (by alpha).

The default sort function needs to be able to handle a registry passed to it, so to make `sorted` the default behavior, we have to create a lambda that strips the registry parameter before calling `sorted`.

Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
@MichaelTiemannOSC MichaelTiemannOSC marked this pull request as ready for review January 23, 2024 09:59
@MichaelTiemannOSC
Copy link
Collaborator Author

I don't know why that one test failed--looks like a CI hiccup.

This is a much more logical place to put it.  Note that the default formatters (plain, html, latex) all now call `formatter` with sort_func=None so that we don't accidentally use `sorted` as a default argument.  But those who call `formatter` directly for their own purposes can call with a sort_func of their choosing that will do what they want it to do.

This also fixes a latent bug where we failed to call `sort_func` in one of the paths of `plain.py`.

Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
@hgrecco
Copy link
Owner

hgrecco commented Jan 24, 2024

I don't know why that one test failed--looks like a CI hiccup.

I can fix that in my computer. But how do you think we should merge #1864 and #1926 (this?) Do want to merge them into one and I merge them?

@MichaelTiemannOSC
Copy link
Collaborator Author

I think this is the one PR to merge (the other is obsoleted by this one). We can certainly add some more tests after merging that fleshes out how complex multi-unit dimensions should be sorted, but that will only change the inner operation of the dim_sort function, not the interfaces (I think).

@hgrecco hgrecco merged commit e3f24ab into master Mar 8, 2024
94 of 97 checks passed
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.

Override units display order (1 kW·h displayed as 1 h·kW)
2 participants