-
Notifications
You must be signed in to change notification settings - Fork 199
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
Fix Latex support #95
Conversation
…alled - This case was generating an empty image instead of using the plain text representation for SymPy objects - It builds on top of ipython/ipython#9203
…nclosed in dollar signs
@@ -140,9 +140,8 @@ def _handle_execute_result(self, msg): | |||
self._pre_image_append(msg, prompt_number) | |||
try: | |||
self._append_latex(data['text/latex'], True) | |||
except Exception: | |||
self.log.error("Failed to render latex: '%s'", data['text/latex'], exc_info=True) |
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.
Isn't it a good thing to log failure to render the latex?
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 think we should keep catching all exceptions and only in case of a ValueError (or probably a custom subclass we define) we swallow the error (as this one means that we got non-equation LaTeX)
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.
Isn't it a good thing to log failure to render the latex?
Not for Spyder and Canopy because those failures are shown in the terminal where those applications were started. If they are started graphically, the errors won't be seen by their users.
Besides, (as I said in ipython/ipython#9203) possible errors are:
- Matplotlib is unable to handle Latex, something not easily discernible from the traceback.
- Latex has syntax errors, something that also generates one of those long and almost unreadable report errors.
So this simplifies things and handles all cases I mentioned at the beginning correctly.
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.
or probably a custom subclass we define
Yes, I was thinking to create a simple LatexError
class.
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.
good points. it’s the latex-repr-writers’ responsibility to emit correct code anyway and if they don’t, they’ll have to debug it.
@asmeurer thanks, following sympy's lead seems sensible here. |
Thanks @asmeurer for the input. I changed the task accordingly. |
OK, then the only thing left is the other mathmode triggers (which only work for the dvipng backend) inline:
display:
|
@flying-sheep, I won't implement all that, for two reasons:
Maybe you could do it after this PR is merged :-) |
Or you could send a PR against my branch if you want it as part of this. |
no separately is OK or let me do it inline |
Against my branch? Please be my guest :-) In any case, thanks for helping me out. |
OK. here it is. is this sufficient or should i do the PR? basic_envs = 'math displaymath'.split()
starable_envs = 'equation eqnarray multline gather align flalign alignat'.split()
star_envs = [env + '*' for env in starable_envs]
envs = basic_envs + starable_envs + star_envs
env_syntax = [r'\begin{{{0}}} \end{{{0}}}'.format(env).split() for env in envs]
math_syntax = [
(r'\[', r'\]'), (r'\(', r'\)'),
('$$', '$$'), ('$', '$'),
]
def is_latex_math(latex):
for start, end in math_syntax + env_syntax:
inner = latex[len(start):-len(end)]
if start in inner or end in inner:
return False
if latex.startswith(start) and latex.endswith(end):
return True
return False |
I think it's enough, thanks a lot! |
NP! i edited it a bit since first commenting, so the version at the time of writing should be good |
@minrk, I think this one is ready. Please review it and let me know what you think. |
great, thanks! |
Fixes #56
This builds on top of ipython/ipython#9203, so that one has to be merged first.
Pinging @flying-sheep, @asmeurer and @jorisvandenbossche
Tasks: