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

Update latexoperation.jl #280

Closed
wants to merge 1 commit into from
Closed

Conversation

co1emi11er2
Copy link

When interpolating Unitful types under an exponent, the latexoperation function should wrap ex.args[2] in parenthesis to properly display equation.

julia> using Unitful, UnitfulLatexify, Latexify
julia> x = 2u"inch"
julia> y = 2u"inch"
julia> @latexdefine z = $x^2 + $y^2
L"$z = 2\;\mathrm{inch}^{2} + 2\;\mathrm{inch}^{2} = 8\;\mathrm{inch}^{2}$"

298393262-bf7c74aa-9f3d-4e6f-83ee-83c5842ae1aa

You can see from the image above that it should look more like: z = (2 inch)^2 + (2 inch)^2 = 8 inch^2

This pull request fixes that issue. It checks whether the ex.args[2] is Real or is a Symbol, and if it isn't one of those, it wraps the ex.args[2] in parenthesis. I realize this may be too broad of a criteria. I didn't want to add any dependencies, but hopefully this pull request opens discussion on how best to fix this. Thanks!

When interpolating Unitful types under an exponent, the latexoperation function should wrap ex.args[2] in parenthesis to properly display equation.
Copy link

codecov bot commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6b869c3) 82.38% compared to head (4a28404) 82.38%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #280   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files          21       21           
  Lines         829      829           
=======================================
  Hits          683      683           
  Misses        146      146           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@co1emi11er2
Copy link
Author

Hi,
I just wanted to bump this pull request and see if anyone had time to review this?
I don't believe the failures are due to any changes I made.
Thanks,

@gustaphe
Copy link
Collaborator

gustaphe commented Apr 9, 2024

Hi!
I'm sorry, I'm writing my thesis, not a lot of time and energy to go around.
My feeling is this might indeed be too broad a distinction - though I can't think of any adverse examples right now.
What should happen is that prevOp should be :* when an AbstractQuantity is latexified in UnitfulLatexify, but I don't think there's currently a method to communicate this from a latexrecipe.
A tandem update of Latexify to include a Expr(:latexifymergeas, :*, ... and UnitfulLatexify to use it would probably be best, but maybe we could do with second best.

@gustaphe
Copy link
Collaborator

gustaphe commented Apr 9, 2024

Actually, I can think of a couple of counterexamples. Arrays for one. Bra-kets is another (that we've used as an example of a latexifiable object before). I'm starting to think quantities are really the outliers in this respect - it would probably not be very good to have the bracketing per default.

@co1emi11er2
Copy link
Author

Yeah I thought of arrays as one that would now be wrapped in parenthesis with my update.
image

I can look into it more. My package Handcalcs.jl just does a lot more interpolation of symbols so with Unitful things just break under exponents. With my update everything works, but things like matrices and anyone elses types may just get that extra parenthesis. I think there is definitely a better solution though!

@TS-CUBED
Copy link

Are there any plans to include something like this in future versions of Latexify? I teach my students to use units in all their calculations, and this little issue trips them up a lot.

I currently use Cole's fix in a deved version of Latexify, but hope to convince other colleagues to move to the new system of creating learning material using Julia, for which this (deving modules) may be a bit too much of a hack.

@gustaphe
Copy link
Collaborator

Yes. I found a bit of time to work on this, #292 does it the way I think it should be done.
It's a really simple change for UnitfulLatexify afterwards -- I already have it ready. Just going to give #292 a couple of days to fester.

@gustaphe gustaphe closed this Jul 2, 2024
@gustaphe
Copy link
Collaborator

gustaphe commented Jul 3, 2024

This is now fixed, rejoice.

@TS-CUBED
Copy link

TS-CUBED commented Jul 3, 2024 via email

@co1emi11er2
Copy link
Author

Thank you for the update! I have now updated my package for the changes as well!

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.

3 participants