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

Algorithmic decorator (implement output for _repr_latex_) #163

Merged
merged 31 commits into from
Dec 15, 2022

Conversation

ZibingZhang
Copy link
Contributor

@ZibingZhang ZibingZhang commented Dec 11, 2022

Overview

Adds a IPythonAlgorithmicCodegen class which converts algorithms into $\LaTeX$ for IPython.

References

continues work on #57

Blocked by

None

@ZibingZhang
Copy link
Contributor Author

ZibingZhang commented Dec 11, 2022

A point of interest is that the result of get_latex(style=EXPRESSION) doesn't return valid $\LaTeX$, while get_latex(style=ALGORITHMIC) does. Feels weird to have this discrepancy, but I'm not sure of a good way to resolve it.

@odashi
Copy link
Collaborator

odashi commented Dec 11, 2022

General comments before providing detailed review:

  • Could you split the pull request into at least 2 separate PRs: introducing IPython-specific codegen and revising frontends?
  • IPythonAlgorithmicCodegen makes sense more than `AlgorithmicJupyterCodegen. _repr_latex_ is the feature of IPython and Jupyter (and other REPL runtimes) borrows this interface.
  • \displaystyle can be avoided at this point.
  • \hspace{} with the em unit is better to follow the size of the characters.

Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dummy, please re-request review after all changes in frontend were split into other PR.

@ZibingZhang
Copy link
Contributor Author

Dummy, please re-request review after all changes in frontend were split into other PR.

I'm not ready for this PR to be reviewed, but #164 is ready for review.

Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use \textsc for small capital letters.
  • Wrap all statements by \begin{array}{l} and \end{array}. newlines are unexpected in the bare math mode and it brings unexpected consequence (e.g., $a\\b$ and $$a\\b$$ may generate different results in terms of the number of lines)

@ZibingZhang
Copy link
Contributor Author

  • Use \textsc for small capital letters.

Tried it in the Google collab notebook and \textsc{fact} dosn't seem to be working

image

Maybe \texttt?

image

@odashi
Copy link
Collaborator

odashi commented Dec 14, 2022

\textsc{fact} dosn't seem to be working

Okay, so I guess \mathrm{...} is eventually better, and I don't agree with any options with manual uppercasing.

@odashi
Copy link
Collaborator

odashi commented Dec 14, 2022

\texttt

No, we don't have any necessity to use typewriter font.

@ZibingZhang
Copy link
Contributor Author

I don't agree with any options with manual uppercasing.

Sounds good, I can remove that. Only had it because ALGORTIHMIC codegen produces \function{fun_name} which auto uppercases.

@@ -26,20 +26,60 @@ def check_function(
if not kwargs:
latexified = frontend.function(fn)
assert str(latexified) == latex
assert latexified._repr_latex_() == rf"$$ \displaystyle {latex} $$"
assert latexified._repr_latex_() == r"$$ \displaystyle " + latex + " $$"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore the original expression, and don't use "+" concatenation as well as possible. f-strings or join() are basically the most fastest option to concatenate strings and "+" is the worst choice.

Ditto for all other places.

def f():
  for i in range(1_000_000):
    x = f"The answer is {i}."
  return x

def g():
  for i in range(1_000_000):
    x = "The answer is " + str(i) + "."
  return x

%time f()
%time g()
CPU times: user 191 ms, sys: 0 ns, total: 191 ms
Wall time: 192 ms
CPU times: user 327 ms, sys: 0 ns, total: 327 ms
Wall time: 327 ms

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, I think I've converted it all back. When codegen though lots of places can't use f strings because of \ not allowed :(

@odashi odashi merged commit ff947f7 into google:main Dec 15, 2022
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.

2 participants