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

Enable changing the font color for LaTeX rendering #11840

Merged
merged 3 commits into from Aug 10, 2019

Conversation

@oscargus
Copy link
Contributor

commented Jul 30, 2019

An issue that shows up in quite a lot of places is that SymPy rendering doesn't consider the background color.

IPython: #9451
Spyder: spyder-ide/spyder#3798
SymPy: sympy/sympy#10655

This PR makes it possible to send along a dvips color name (default is black), which will make it a bit easier for all these tools to do something clever.

One could considering adding an "auto" default option as well, see sympy/sympy#17244, but it wasn't clear if it was a good idea and/or possible to get the colors variable from lib.latextools (I imagine it is sometimes used standalone?). This would effectively (or at least in a statistical sense) close #9451

If it makes sense, I can easily add a scale keyword argument as well to deal with high dpi screens. SymPy just got one: sympy/sympy#17298

@oscargus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Oh, it wasn't really clear what to test. But at least some exercise is done to see that the results are consistent and that dvips name can be used. (I have manually confirmed if for matplotlib.)

@oscargus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Clearly, there is no latex installed in the test environment...

Ahh, I saw the markers now... Will add those.

@Carreau

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Hi ! Thanks for the the pull-request;

We should have a better story to skip test with latex. Worst cas, just comment the test; this look strait forward enough.

@oscargus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I've just added a PR in SymPy that includes a color representation converter: sympy/sympy#17319

Maybe it makes sense to use that here as well? If not, one problem facing a user is that colors are (except for the 68 dvips colors) specified differently depending on which backend is used. And as one aspect is that one shouldn't have to worry about that when calling latex_to_png, it seems like the option otherwise is to provide two keyword arguments, one for color in case dvipng is used, one for color in case matplotlib is used. (I haven't really gotten into how to possibly use other backends.)

Also, for e.g. qtconsole/Spyder it is probably easier to provide colors in hex rgb than looking up the closest dvips color name. So unless you have some insights why I shouldn't do it, I aim to include the color conversion here as well (unfortunately the color conversion in Matplotlib doesn't play well with the LaTeX special color format, so another option is of course to change that so it can handle numerical LaTeX colors...).

@oscargus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Of course, I can add a subset of the conversions here. Just enough to get from hex rgb to something that LaTeX/dvipng can handle. In that case one can either specify a named LaTeX color or as hex rgb. Should probably be enough for most people. (SymPy users can still specify using any of the numerical LaTeX color formats.)

@oscargus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Hmm, appears as if I made some small changes anyway.

It is now possible to provide hex RGB colors as well (which are converted if dvi png is used). I also added a scale optional keyword to (yes!) scale the output. Finally, I ead the dvipng manual: "-x
num This option is deprecated; it should not be used. It is much better to select the
output resolution directly with the -D option." So I used the -D option.

@Carreau

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Thanks for your patience, i've been quite busy.

This looks good to me, so I'm happy to get that in. It also works quite well with a current in progress feature that allow to display inline images in terminal that support them:

Screen Shot 2019-08-07 at 6 28 38 PM

@Carreau Carreau added this to the 7.7 milestone Aug 8, 2019

@ccordoba12

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Carreau, thanks a lot for taking a look at this one! We need this in Spyder to correctly adjust Sympy equation colors in our consoles.

When do you plan to release a new IPython version that includes this fix?

@oscargus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@Carreau No problem! It's been an issue for years, so a week or two is no big deal. :-) Do you think it may be possible/wanted to detect the IPython colors variable from here? Running SymPy from a plain ipython shell will probably not benefit from it (but then I am not certain at all how things are interconnected). Although, e.g., qtconsole will as they can set it based on the background color knowledge.

@ccordoba12 It is worth noting that running init_printing in SymPy (which Spyder does for a SymPy shell) will have SymPy override this, as it will send a PNG which qtconsole will show instead of rendering its own. However, it can be sorted by providing the color to init_printing. Still, in a plain console, before possibly executing init_printing, it will definitely help.

@ccordoba12

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

However, it can be sorted by providing the color to init_printing

Yes, that's exactly what we need to do! However, we need to communicate the kernel if the console background color is dark or light (and if there are changes to it), and then re-execute init_printing accordingly.

That's our next task after spyder-ide/spyder#9343 is merged, which will make that process very simple.

@@ -10,6 +10,7 @@
import shutil
import subprocess
from base64 import encodebytes
from textwrap import wrap as splitstring

This comment has been minimized.

Copy link
@ccordoba12

ccordoba12 Aug 8, 2019

Member

This is kind of non-standard, so I'd prefer if you use simply use wrap here instead of using an alias.

This comment has been minimized.

Copy link
@Carreau

Carreau Aug 9, 2019

Member

The issue is there is a conflicting keyword named wrap, where this is used. I've pushed a commit that use the fully qualified name.

This comment has been minimized.

Copy link
@oscargus

oscargus Aug 12, 2019

Author Contributor

Thanks @Carreau ! Didn't really take a step back and figured out a better solution, but was just trying to get it to work. Clearly, using a fully qualified name is the way to go.

@Carreau Carreau modified the milestones: 7.7, 7.8 Aug 8, 2019

@Carreau

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

When do you plan to release a new IPython version that includes this fix?

I do a minor release every last Friday of the month (when I can), this is likely to make the cut to this month.

Do you think it may be possible/wanted to detect the IPython colors variable from here

Let's do that in a further iteration. I'm planning on adding a text/latex handler to IPython itself that could then decide on the color and that would alleviate sympy's needs to set the color or generate PNG, and then the frontend can just pickup the math in display in whatever color they wish.

Use explicit fully qualified name instead of uncommon alias.
We can't use 'wrap' as there is a kwarg with the same name.
@Carreau

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Strange, I've push but github don't show it:

$ git push
Enumerating objects: 9, done.
Counting objects: 100% (9/9), done.
Delta compression using up to 8 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 529 bytes | 529.00 KiB/s, done.
Total 5 (delta 4), reused 0 (delta 0)
remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
To ssh://github.com/oscargus/ipython.git
   978f4da6a..e5dc064ee  enablelatexfontcolor -> enablelatexfontcolor
@Carreau

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I'm going to close and reopen see if it sees the new commits.

@Carreau Carreau closed this Aug 9, 2019

@Carreau Carreau reopened this Aug 9, 2019

@Carreau

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Ok, it worked. CI need to re-run though.

@ccordoba12
Copy link
Member

left a comment

Thanks @oscargus !

@Carreau Carreau merged commit fe0b87c into ipython:master Aug 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.