-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Latexbranch #464
base: main
Are you sure you want to change the base?
Latexbranch #464
Conversation
Codecov Report
@@ Coverage Diff @@
## master #464 +/- ##
==========================================
- Coverage 96.47% 93.51% -2.97%
==========================================
Files 35 35
Lines 1617 1680 +63
==========================================
+ Hits 1560 1571 +11
- Misses 57 109 +52
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening the PR. Looks really interesting. Regarding the font sizes. Do you know why yours are bigger than before?
I just had a look through the code so not a full review yet but maybe something of help. Will try a full review today or tomorrow.
I think you don't have a description of how to run it with the new way yet, right? Do you mind to write down a step by step instruction in the PR for people to just try it out including how to install it. We can later put that in the docs then.
src/latex.jl
Outdated
end | ||
LaTeXSVG[text] = svg | ||
end | ||
return svg | ||
end | ||
|
||
function tex2svg(text::LaTeXString; output_dir = "./.TeXfolder") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this output dir works for windows systems. I think there is a Julia function to create a temporary folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes mktemp
and mktempdir
, will be a neater way to do this ;
will change that .
src/latex.jl
Outdated
generates svg from TeX; | ||
default folder placed is ./.TeXfolder, | ||
pdf's and aux files are deleted , tex and log files and other files remain | ||
add LaTeX packages in `packages=[]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the comment. Is there a reason why they don't get deleted?
Also what do you mean with packages =[]
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no reason to keep the pdf's around after we're done converting to svg;
i just kept it around incase people want to inspect the log and tex files if they do run into errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages=[]
was an earlier way i was passing the packages to be added in the usepackage line of the .tex file ; ive changed it in a later commit to a module level variable Javis.LaTeXusepackages
which is a vector holding all the package names.
will remove the add Latex package in ...
line
end | ||
LaTeXSVG[text] = svg | ||
end | ||
return svg | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to have doctrings here and not inside the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ! Pythonic habits die hard ; will fix that !
src/svg2luxor.jl
Outdated
defs = Dict{String,Any}() | ||
for def in collect(child_elements(def_element)) | ||
defs[attribute(def, "id")] = def | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each try catch I would like to know why it can fail and maybe there is an option to avoid the try catch then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This try statement checks if we have any def
tags , if they exist we add the defs to defs
dictionary
if there are no def
tags ( like in the case of dvisvgm which draws directly and doesnt predefine
paths to use) get_elements_by_tagname(xroot, "defs")
will return an empty iterable and cause a bounds error .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! i probably shouldn't have discarded all errors , should throw(e)
if its not a BoundsError
;
but a try
free way would be better , the right way would be to check the lengths and proceed
all_def_tags = get_elements_by_tagname(xroot,"defs")
if length(all_def_tags) !=0
defelements = all_def_tags[1]
...
...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes some kind of !isempty
would be nicer than a try
.
yes , i haven't documented how to use dvisvgm over the default , I'll add that. As for the font sizes , beats me ! as for the scaling factor i think iv'e got it right . At the risk of sounding like a bad worker who blames their tools |
its not really mathjax's fault , it Javis scaling mathjax fonts the wrong way , thats what |
Thanks for checking that out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know when you have an example for me to try it out. 😉
I've added some small comments for things I noticed
src/svg2luxor.jl
Outdated
@@ -91,6 +102,7 @@ function draw_obj(::Val{:path}, o, defs) | |||
for pi in 1:length(data_parts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid using pi
src/svg2luxor.jl
Outdated
elseif command == 'Z' | ||
closepath() | ||
else | ||
@warn "Couldn't parse the svg command: $command" | ||
end | ||
end | ||
|
||
if stroke_width != nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with nothing
should use triple equal sign or in this case !==
.
src/svg2luxor.jl
Outdated
defs = Dict{String,Any}() | ||
for def in collect(child_elements(def_element)) | ||
defs[attribute(def, "id")] = def | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes some kind of !isempty
would be nicer than a try
.
Regarding the font size I'm super happy you figured it out way more than I did 😉 |
@Wikunia , added an example gist in the PR :) |
With the gist you mentioned I'm getting the following warning:
with this file: https://gist.github.com/Wikunia/75b74eb6806a9fac10b52fc676c0198c not sure whether this is expected. |
Thats a small bug ; line 208 in latex.jl (in latexbranch) should be Also the tests fail now because ive changed scaling to 425/1000; lemme know if i |
…rning of failed latexpdf generation , even even latex returned 0
PR Checklist
primarily this PR adds support for using dvisvgm which is distributed with TeXLive to render latex . This is an alternative to the existing mathjax solution. Mathjax still exists as the default setting for now.
All tests dont pass because the images are not exact 1-1 pixel replicas .. but they are close enough
edit1: Former are from dvisvgm and latter are from mathjax tex2svg
Linux: To get this working you will need
latex
(symlinked to pdftex binary) anddvisvgm
in your path, pdftex, thelatex
symlink, and dvisvgm are made available by installing TeXLive (most distributions package TeXLive in their repos)To try out an example program
https://gist.github.com/ArbitRandomUser/da8d439544945466c1df566df561d5f4
Windows & MacOS: somebody's gotta test this for me , i dont have access to either OS's at the moment.
(by default \begin{equation} adds numbering in latex)
![act_matrix_transpose1](https://user-images.githubusercontent.com/58146965/153086309-01b19021-1ce5-48ee-93cd-3f254cf7d3bd.png)
![ref_matrix_transpose1](https://user-images.githubusercontent.com/58146965/153086314-9cfc9330-17b3-4432-a5cb-72776f9dbe5a.png)
If you are contributing to
Javis.jl
, please make sure you are able to check off each item on this list:CHANGELOG.md
with whatever changes/features I added with this PR?Project.toml
+ set an upper bound of the dependency (if applicable)?test
directory (if applicable)?Link to relevant issue(s)
Closes #
How did you address these issues with this PR? What methods did you use?