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

Let latexify decide on cdot #794

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

Conversation

baggepinnen
Copy link
Collaborator

cdot --> false leads to very confusing output, like the one below, is there a variable called $adelay$ or is it $a \cdot delay$?
image

The example comes from https://mtk.sciml.ai/dev/basics/Composition/

In this case, the correct answer is $a \cdot delay$, but if there was actually a variable called $adelay$ present in the system, the printed output would be incorrect.

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #794 (fc3216d) into master (3bad882) will decrease coverage by 73.36%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #794       +/-   ##
==========================================
- Coverage   76.41%   3.05%   -73.36%     
==========================================
  Files          26      26               
  Lines        3235    3142       -93     
==========================================
- Hits         2472      96     -2376     
- Misses        763    3046     +2283     
Impacted Files Coverage Δ
src/latexify_recipes.jl 0.00% <ø> (-52.78%) ⬇️

... and 24 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@isaacsas
Copy link
Contributor

Using cdot in printing a symbolic expression seems unusual for CAS systems. Maybe this should be controlled in ModelingToolkit via a system field that the corresponding recipe there can look at? I’ve found the opposite experience in reaction networks with many species, where the presence of lots of cdots can also make the output hard to read, so being able to turn this on and off for systems would be nice.

@baggepinnen
Copy link
Collaborator Author

baggepinnen commented Nov 22, 2022

The problem is that our current latexification is not smart enough to distinguish

julia> @variables d x dx;

julia> latexify(dx + d*x)
L"\begin{equation}
dx + d x
\end{equation}
"

$dx + d x$

which is a quite severe correctness issue in the printing. Unless we manage to sort out and prevent this, we should really go for the correct, albeit slightly uglier, option as default.

Using cdot in printing a symbolic expression seems unusual for CAS systems.

SymPy does the correct thing here and prints the dot

julia> @vars d x dx
(d, x, dx)

julia> dx + d*x
dx + dx

@isaacsas
Copy link
Contributor

isaacsas commented Nov 22, 2022

I'd suggest inserting a space via \, instead. That is what, for example, Mathematica does., and much more in line with what someone would do when writing a paper in Latex.

@isaacsas
Copy link
Contributor

isaacsas commented Nov 22, 2022

Alternatively I guess one could use {\cdot} which should avoid the extra spacing introduced by \cdot itself. That looks consistent with the spacing being introduced in SymPy in the terminal display.

@baggepinnen
Copy link
Collaborator Author

I find both of these suggestions okay, as long as one can distinguish a variable name with multiple characters from multiplication

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.

4 participants