-
Notifications
You must be signed in to change notification settings - Fork 156
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
Let user hook into variable Latexifying #1350
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1350 +/- ##
=========================================
+ Coverage 3.98% 6.47% +2.49%
=========================================
Files 50 50
Lines 4771 4772 +1
=========================================
+ Hits 190 309 +119
+ Misses 4581 4463 -118 ☔ View full report in Codecov by Sentry. |
I had been thinking we could just add a kwarg to Symbolics.jl/src/latexify_recipes.jl Lines 131 to 135 in 8c518c2
work, one would need to effectively change a global variable. But I think that even then it makes sense to have a That struct/Dict could, for example, store the |
I don't think that would work because if you all pirate the same method, then you just get the behavior of the last package imported. Piracy is also not recommended. |
That makes sense. Thanks for mentioning it quickly. |
What if we add a symbolic metadata that when set stores the Symbolics.LatexConfiguration object to use for a given symbolic variable? (If not set, the default is used.) |
That avoids needing a global variable and potentially allows per-variable control on formatting. If we make the struct mutable then it should just be storing a pointer right, so not be too heavy? |
Something like |
# in Symbolics.jl we have
struct LatexConfiguration{F}
latexify_variables::F
end
# in your code
lc = Symbolics.LatexConfiguration(pass_your_custom_function)
@variables money [latex_config = lc] |
I could see that working. But if we tie Latex formatting to each variable, is there any gain in using a function? Would not an (optional) per-variable string be simpler and accomplish the same? Or should performance be significantly better by only storing a function? If formatting is not tied to each variable, I fully see the need for a function, though. I'm torn on this 😅 Catch-all formatting functions are great at making systematic adjustments to all variables. And per-variable formatting is great for tuning specific variables only. Maybe both options could even be used in conjunction? I almost have something working that uses |
The problem is that ModelingToolkit can change variable names under the hood when it namespaces. Then the attached metadata would no longer be the right latex string i.e. Dispatches on the recipes is what I thought originally too, but I don't see how to get them to work with |
Could you not define a |
Indeed. I guess MTK could respect the Latex strings by concatenating them when namespacing variables. But a function would also handle it, as you say. |
But that will only give correct displays when at some top level one is calling Symbolics.jl/src/latexify_recipes.jl Lines 131 to 135 in 8c518c2
|
You are right. I think that is a killer argument. TL;DR: one way or another, formatting must be tied to each variable. |
There has been some changes/discussion about how variables are Latexified in JuliaSymbolics/SymbolicUtils.jl#659, JuliaSymbolics/SymbolicUtils.jl#660, #1314 and #1305.
This is a suggestion to provide a hook
Symbolics.latexify_variables
that users (e.g. ModelingToolkit, Catalyst, other packages or humans) can redefine to customize this. In a fresh Julia session, it could work like this:If ModelingToolkit gets something like a
LatexDisplayOptions
object in the future, my idea is that it could use this hook to do its thing. But for many packages, maybe it is sufficient to just override this function, based on what types of equations and variables it has.Any thoughts on this approach and potential issues? cc @ChrisRackauckas @isaacsas