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 Latexify extension #668

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gustaphe
Copy link
Contributor

#665

This is not quite done yet. Because of how extensions work I had to break two things from UnitfulLatexify.

  1. Instead of i.e. latexslashlabel(s, u) it's latexify(s, u; labelformat=:slash). This is a better interface anyway, I've been meaning to change it since the start but didn't want to break it. As far as I can tell I can't export those functions from the extension in a meaningful way, so maybe this is just the time. I don't think those functions are very common anyway.
  2. This one is the one I primarily want input on: u"One". UnitfulLatexify had u"one" as a non-fragile version of NoUnits, so you could have numbers that are recognizably unitful but with units of 1. AFAICT, I cannot currently register units in an extension (I tried a couple of things, but nothing quite stuck), so I needed to register this unit in Unitful. But because Unitful imports Base.one I needed to change the spelling. Perhaps the right way is to fix registering units in an extension (more people might need that), but I don't know where to start.

@sostock
Copy link
Collaborator

sostock commented Jul 31, 2023

This is not quite done yet. Because of how extensions work I had to break two things from UnitfulLatexify.

  1. Instead of i.e. latexslashunitlabel(s, u) it's latexify(s, u; labelformat=:slash). This is a better interface anyway, I've been meaning to change it since the start but didn't want to break it. As far as I can tell I can't export those functions from the extension in a meaningful way, so maybe this is just the time. I don't think those functions are very common anyway.

Are these functions (latexslashunitlabel etc.) actually meant to be called by the user (they don’t appear in the documentation), or are they just helper functions that are used by plot(...; unitformat=latexslashunitlabel)? I am asking because in the latter case, I wouldn’t even consider this breaking (IMO, if latexslashlabel is considered an internal function, it can be removed at any time). Anyway, I like this change.

  1. This one is the one I primarily want input on: u"One". UnitfulLatexify had u"one" as a non-fragile version of NoUnits, so you could have numbers that are recognizably unitful but with units of 1. AFAICT, I cannot currently register units in an extension (I tried a couple of things, but nothing quite stuck), so I needed to register this unit in Unitful. But because Unitful imports Base.one I needed to change the spelling. Perhaps the right way is to fix registering units in an extension (more people might need that), but I don't know where to start.

Here are my thoughts, but keep in mind that I’m not really familiar with Latexify and UnitfulLatexify:

To me, it looks like the reason for having the u"One" unit is to enable using siunitx’s \num{…} for plain numbers. I think using the abilities of \num{…} is a useful feature even for users who do not work with units at all, so I think it should just be part of Latexify itself 1. Latexify already has the fmt keyword argument that takes arguments of type AbstractNumberFormatter, so we could propose to add a type SiunitxFormatter <: AbstractNumberFormatter to Latexify that simply prints numbers with \num{…}. I think that would remove the need for the u"One" unit, or is there some other use for it?

Another question to consider: When this is merged and released, and users of UnitfulLatexify upgrade Unitful without making changes to their code, it will load both UnitfulLatexify and this extension (I think). Will that lead to problems? I haven’t tried it yet, but I think we should make sure that upgrading Unitful does not break the code of existing UnitfulLatexify users.

Footnotes

  1. The fact that it doesn’t really have anything to do with units leads me to the same conclusion.

@gustaphe
Copy link
Contributor Author

gustaphe commented Aug 1, 2023

Thank you for the comments.

are they just helper functions that are used by plot(...; unitformat=latexslashunitlabel)?

This, but that is a user facing interface. I would not be surprised if few to no people ever used them though. Users going from UnitfulLatexify to LatexifyUnitfulExtension will have to either change plot(...; unitformat=latexslashunitlabel) to plot(...; unitformat=(s,u) -> latexify(s, u; labelformat=:slash)) or better yet use set_default(labelformat=:slash); plot(...;, unitformat=latexify). It's a good change though.

add a type SiunitxFormatter <: AbstractNumberFormatter

That's not a half bad idea. It could even supersede the entire unitformat argument (in Latexify, not in Plots ...). I've been blind to that option, will need to spend a bit of time thinking about it but it's probably a good idea.

Another question to consider

Yes, this is going to take a bit of thinking. When we merged UnitfulRecipes into Plots we introduced a warning -- I'll look into how that was done. There was a bit of compatibility hickups at first, but hopefully we learned the lesson.

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.

2 participants