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

Only use automatic unit conversion for simple units (not compound units) #4583

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Nov 11, 2024

Description

This pr removes code attempting to find the best units for the given user input/limits. As noted in #4063 this code (always?) leads to incorrect units for compound units, e.g. u"m/s". It also seems like a difficult problem to determine how to distribute prefixes or if they are even wanted.

After a brief discussion with Simon I changed this pr to only affect compound units, i.e. units like u"m/s". These units now no longer try to find an optimal prefix, which led to them becoming wrong. They instead get set once and then stay what they are. Simple units like u"m" continue to optimize their prefix.

The reason for the change in direction is that you can already turn off the prefix conversions by creating a UnitfulConversion with the unit you want. E.g.

uc = Makie.UnitfulConversion(u"m")
scatter(1:4, (10:10:40) .* u"km"; axis=(dim2_conversion=uc,))

Results:

  • compound units are no longer simplified, e.g. (1:10) .* u"nm/m^2 results in unit u"nm/m^2
  • prefixes of compound units are no longer adjusted, e.g. 1e6 u"nm/m^2" keeps u"nm/m^2"
  • if prefixes are changed from plot to plot, the initial prefixes are used for compound units. E.g. if the first plot uses 1200u"nm/m^2 " and the second 1.3u"µm/m^2", u"nm/m^2" will still be used.

I also added some extra code to convert a unit string with exponents to rich text with superscript.

Added refimg:

begin
    # Don't swallow units past the first
    f, a, p = scatter((1:10) .* u"J/s")
    # Don't simplify (assume the user knows better)
    scatter(f[1, 2], (1:10) .* u"K", exp.(1:10) .* u"mm/m^2")
    # Only change prefixes of simple units, not compound units
    scatter(f[2, 1], 10 .^ (1:6) .* u"W/m^2", (1:6) .* 1000 .* u"nm")
    # Only change units/prefixes for simple units when adding more plots
    scatter(f[2, 2], (0:10) .* u"W/m^2", (0:10) .* u"g")
    scatter!((0:10) .* u"kW/m^2", (0:10) .* u"kg")
    f
end

image

Closes #3891
Fixes #4063

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@MakieBot
Copy link
Collaborator

MakieBot commented Nov 11, 2024

Benchmark Results

SHA: 956ff6e8edc97577d2a5c4a45b73c35fb15bb7ad

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@ffreyer ffreyer changed the title Remove automatic unit conversions Only use automatic unit conversion for simple units (not compound units) Nov 12, 2024
@ffreyer
Copy link
Collaborator Author

ffreyer commented Nov 12, 2024

Maybe slightly breaking for units with powers because of rich text addition. I.e. u"m^3" should now render as m³ instead of m^3

@SimonDanisch
Copy link
Member

u"m^3" should now render as m³ instead of m^3

I wouldn't consider this as breaking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready to review
Development

Successfully merging this pull request may close these issues.

Wrong units Disable Automatic Unit Conversion
3 participants