Generates algorithmic environment from algpseudocode #152

ZibingZhang · 2022-12-06T20:52:56Z

Overview

Generates algorithmic environment from algpseudocode.

Details

Creates a new class algorithm_codegen that should generate $\LaTeX$ code in the following form:

Example

    def collatz(n):
        iterations = 0
        while n > 1:
            if n % 2 == 0:
                n = n / 2
            else:
                n = 3 * n + 1
            iterations = iterations + 1
        return iterations

\begin{algorithmic} ... \end{algorithmic}

Things to take into consideration:

The configs won't be the same for this and function_codegen, at least for the time being. Namely use_signature will be disregarded if set.
For the time being, this functionality is exposed through get_latex(style=Style.ALGORITHM, but there should be a way to use it as @latexify.algorithm. But what should the return type be? str? LatexifiedFunction?
There are multiple algorithmic environments, how should be specify which ones to target? Which ones should be supported?

References

Described by this issue: #57

Blocked by

None

ZibingZhang · 2022-12-06T22:42:34Z

@odashi let me know what you think about this approach. Looking for a bit of early feedback before continuing.

odashi · 2022-12-07T04:54:30Z

@ZibingZhang Could you open the pull request instead of leaving it as a draft? I'm using PR's status to manage my taskboard and sometimes draft PRs are overlooked.

odashi · 2022-12-07T05:05:52Z

Thanks! I will take a look at the code later today. Some comments about the description:

else n % 2 == 1:

This may be a typo.

@latexify.algorithm

If we provide this function, it must provide a way to correctly print the expression onto Jupyter. Since algorithm environmets are not supported by Jupyter (MathJax), we need to simulate it by ourselves.

This means that the algorithm version of "LatexifiedFunction" needs two codegens for __str__ and _repr_latex_, but this is far beyond the scope of this PR IMO.

Which environment

algorithmic is fine I think. It is okay to start considering to support other envs when they are actually requested.

At this point, I guessed the enum value should be ALGORITHMIC rather than ALGORITHM to avoid confusion in the future.

src/latexify/codegen/algorithm_codegen.py

ZibingZhang · 2022-12-07T06:25:59Z

used internally only when we need to delegate the codegen logic to it

I'm not sure if this is possible, due to the recursive nature of ASTs. If you look at the first commit I had something like

def visit_SomeNode(node):
    return self._function_codegen.visit(node)

But something of this nature won't work since recursive calls will end up in the FunctionCodegen methods instead of back in the AlgorithmCodegen methods.

I think to avoid the weird is, relationship, they both need to inherit from some private base class which implements the codegen of basic expressions. I'm not sure if we can use composition here, at least I'm not able to think of a way to get it working.

odashi · 2022-12-07T07:57:47Z

I'm not sure if this is possible

It should work as far as the subtree processing is completely delegated. Since both NodeVisitor and our FunctionCodegen are stateless and don't change the given AST, we can invoke visit as a usual function.

Unfortunately there is no private inheritance in Python, and many techniques available in other languages (such as C++) can't be used.

ZibingZhang · 2022-12-07T08:34:18Z

It should work as far as the subtree processing is completely delegated. Since both NodeVisitor and our FunctionCodegen are stateless and don't change the given AST, we can invoke visit as a usual function.

Consider visit_Attribute:

class ACodegen:
    def visit_Attribute(self, node: ast.Attribute) -> str:
        vstr = self.visit(node.value)
        astr = self._identifier_converter.convert(node.attr)[0]
        return vstr + "." + astr
    
    def visit_If(self, node: ast.If) -> str:
        raise Error("Unsupported")

class BCodegen:
    def visit_Attribute(self, node: ast.Attribute) -> str:
        return self._a_codegen.visit(node)

    def visit_If(self, node: ast.If) -> str:
        return "..."

If we have some structure like:

{ "attribute": { "value": { "if": "..." } }

The call stack would look like

BCodegen.visit_Attribute 
-> ACodegen.visit_Attribute
-> ACodegen.visit_If
-> Error

whereas the desired behavior would be

BCodegen.visit_Attribute 
-> ACodegen.visit_Attribute
-> BCodegen.visit_If

Basically any visit_Node function with a self.visit wouldn't work properly when delegated to because self refers to the delegated class.

edit: I guess this could be solved like so?

class BCodegen:
    def visit_Attribute(self, node: ast.Attribute) -> str:
        return ACodegen.visit_Attribute(self, node)

but at that point you may as well just use inheritance, since you're relying on ACodegen and BCodegen having the same type, and instance / class variables.

odashi · 2022-12-07T09:01:44Z

Before continuing discussion, we need to understand how the Python AST is constructed. Overall, all ASTs look like as follows:

[mod subtree] -> [stmt subtree] -> [expr subtree]

where stmt subtree never generate mod and expr never generate stmt. (the official guidance)

In FunctionCodegen and AlgorithmCodegen, we need to handle stmt and expr to generate the final LaTeX, but once the codegen walks into expr, the subroutine processes only expr and it never meet any stmt branch in this recursion. That means we can completely separate expr processing into another library, and this is why I proposed ExpressionCodegen in #57.

In this PR, AlgorithmCodegen need to implement every visitor for stmt nodes by itself, but can delegate every processing for expr to another. Since the current codebase implements every rule in FunctionCodegen, we can simply delegate everything to it, and we don't need additional information to invoke expr visitors in FunctionCodegen.

odashi · 2022-12-07T09:09:45Z

The contrast between stmt and expr is important I guess, because codegen rules to control the overall style (the appearance of the overall algorithm) is basically implemented by only stmt rules.

ZibingZhang · 2022-12-07T09:11:38Z

That makes sense ty for explaining!

The only issue I can maybe see down the line if AlgorithmCodegen decides that an expression should take one form but FunctionCodegen decides it should take another, but I can't think of any examples off the top of my head, so maybe it's a non-issue.

ZibingZhang · 2022-12-07T15:20:42Z

visit_comprehension has to be in ExpressionCodegen for some reason (didn't really look into it), but it's an ast.stmt.

Do you think it's possible to merge this early somehow (i.e. without completing AlgorithmicCodegen). Either by

merging as is (following review & edits) and following up with a PR to expand upon the algorithmic codegen while adding tests
splitting this PR into 2, first part just factoring out ExpressionCodegen, next part the AlgorithmicCodegen implementation

Just worried about trying to deal with future merge conflicts, since this change is changing a fundamental part of the pipeline, and is touching a bunch of files.

odashi · 2022-12-08T00:18:48Z

but it's an ast.stmt

I think it's AST, not stmt. This is because the class is used to aggregate a set of rules (in this case, a sub-expression of a comprehension in some other expr), and appears only within a limited context. This kind of subtree can't be treated as an expr in the AST, e.g. a + <comprehension> is illegal.

We should be able to treat this kind of subtrees safely as a part of either stmt or expr because they shouldn't violate the entire rule of the AST: stmt -> expr. In this case it is okay to assume that this is expr.

odashi · 2022-12-08T00:20:12Z

ExpressionCodegen should be separated into other pull request since this is an independent feature.

ZibingZhang · 2022-12-08T00:20:49Z

ExpressionCodegen should be separated into other pull request since this is an independent feature.

Sounds good. Will tackle this within the next few days!

odashi · 2022-12-08T00:59:20Z

Thanks! It looks your repository breaks the blame history when splitting FunctionCodegen. Since it makes debugging hard, make sure that the history is preserved appropriately:
https://stackoverflow.com/questions/3887736/keep-git-history-when-splitting-a-file

odashi

Dummy, please re-request afterwards

src/integration_tests/integration_utils.py

ZibingZhang · 2022-12-10T08:41:42Z

src/latexify/codegen/algorithmic_codegen_test.py

+        node = ast.parse(
+            textwrap.dedent(
+                """
+                while True:


Fixed whitespacing issues in function_codegen_test.py to match this style, don't know what I was doing when I wrote those tests...

src/latexify/codegen/algorithmic_codegen.py

src/integration_tests/algorithmic_style_test.py

odashi · 2022-12-10T09:16:18Z

src/integration_tests/algorithmic_style_test.py

+
+from typing import Any, Callable
+
+from latexify import frontend


I think this file should be a unit test frontend_{algorithmic?}_test.py because frontend is not intended to be used directly.
Or it'd be okay to use just exported functions and Style.

I can move it, but function_expansion_test and regression_test basically use frontend directly, as they use frontend.function.

Similarly, this file uses function.get_latex. Both of these functions are exported in __init__.py, so it feels somewhat like an integration test, because we're testing end-to-end behavior from function to generated $LaTeX$ output

So it turns that the issue comes from the old implementation of these tests, will consider it later.

src/integration_tests/algorithmic_style_test.py

odashi · 2022-12-10T09:49:41Z

src/latexify/codegen/algorithmic_codegen.py

+        """Visit a While node."""
+        if node.orelse:
+            raise exceptions.LatexifyNotSupportedError(
+                "Codegen does not support while statements with an else clause."


Tip: Users don't usually understand what "codegen" (or any other inner routines) is, so simplifying the message is more user-friendly.

Suggested change

"Codegen does not support while statements with an else clause."

"While statement with the else clause is not supported."

That's fair, was trying to model it off the error message in function_codegen about only having assign nodes in multiline functions. Unsure how to change that message as well to be more user friendly

src/latexify/codegen/algorithmic_codegen_test.py

src/latexify/frontend.py

odashi · 2022-12-10T10:00:58Z

src/latexify/frontend.py

@@ -173,7 +190,7 @@ def expression(
    This function is a shortcut for `latexify.function` with the default parameter
    `use_signature=False`.
    """
-    kwargs["use_signature"] = kwargs.get("use_signature", False)
+    kwargs["style"] = Style.EXPRESSION


This function no longer need to invoke function, just invoking get_latex directly is fine.

expression returns a LatexifiedFunction, while get_latex returns str. I think we still need the extra work that function provides to wrap the result of get_latex, no?

Ah sorry I was a bit confused! I will consider these implementations later.

odashi

Maybe good to merge, thanks!

odashi · 2022-12-10T11:46:35Z

src/integration_tests/algorithmic_style_test.py

+
+from typing import Any, Callable
+
+from latexify import frontend


So it turns that the issue comes from the old implementation of these tests, will consider it later.

odashi reviewed Dec 7, 2022

View reviewed changes

src/latexify/codegen/algorithm_codegen.py Outdated Show resolved Hide resolved

ZibingZhang marked this pull request as ready for review December 7, 2022 06:33

ZibingZhang requested a review from odashi December 7, 2022 15:21

ZibingZhang mentioned this pull request Dec 8, 2022

Factor out expression codegen from function codegen #155

Merged

odashi reviewed Dec 10, 2022

View reviewed changes

init alg

4ada4b5

ZibingZhang force-pushed the algorithm-codegen branch from ff276d7 to 4ada4b5 Compare December 10, 2022 07:49

Zibing Zhang added 3 commits December 10, 2022 07:53

comments and annotations

abac34b

fix indentation

c0e4b27

tests, procedure

bbd3f74

ZibingZhang commented Dec 10, 2022

View reviewed changes

src/integration_tests/integration_utils.py Show resolved Hide resolved

forgot one

349464b

ZibingZhang commented Dec 10, 2022

View reviewed changes

Zibing Zhang added 2 commits December 10, 2022 08:42

expose Style, tests

af7a3c8

bug

8c91639

ZibingZhang commented Dec 10, 2022

View reviewed changes

src/latexify/codegen/algorithmic_codegen.py Show resolved Hide resolved

ZibingZhang commented Dec 10, 2022

View reviewed changes

src/integration_tests/algorithmic_style_test.py Outdated Show resolved Hide resolved

ZibingZhang requested a review from odashi December 10, 2022 08:56

Zibing Zhang added 4 commits December 10, 2022 09:16

rm visit_Expr from expr_codegen

280f416

rm line

0c00706

inline some code

a66f8af

specify codegen

119de91

odashi reviewed Dec 10, 2022

View reviewed changes

too long

5841f93

ZibingZhang mentioned this pull request Dec 10, 2022

Fine-grained control over function name replacements #160

Open

Zibing Zhang added 2 commits December 10, 2022 10:26

suggestions

3634a67

rm todo

628a2da

odashi approved these changes Dec 10, 2022

View reviewed changes

odashi merged commit 93068ef into google:main Dec 10, 2022

ZibingZhang mentioned this pull request Dec 11, 2022

MatchOr and guard statements #158

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generates algorithmic environment from algpseudocode #152

Generates algorithmic environment from algpseudocode #152

ZibingZhang commented Dec 6, 2022 •

edited

Loading

ZibingZhang commented Dec 6, 2022

odashi commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022

ZibingZhang commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

ZibingZhang commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

ZibingZhang commented Dec 7, 2022

ZibingZhang commented Dec 7, 2022

odashi commented Dec 8, 2022 •

edited

Loading

odashi commented Dec 8, 2022

ZibingZhang commented Dec 8, 2022

odashi commented Dec 8, 2022 •

edited

Loading

odashi left a comment

ZibingZhang Dec 10, 2022

odashi Dec 10, 2022

ZibingZhang Dec 10, 2022

odashi Dec 10, 2022

odashi Dec 10, 2022

ZibingZhang Dec 10, 2022

odashi Dec 10, 2022

ZibingZhang Dec 10, 2022

odashi Dec 10, 2022

odashi left a comment

odashi Dec 10, 2022


		from typing import Any, Callable

		from latexify import frontend

	"Codegen does not support while statements with an else clause."
	"While statement with the else clause is not supported."

Generates algorithmic environment from algpseudocode #152

Generates algorithmic environment from algpseudocode #152

Conversation

ZibingZhang commented Dec 6, 2022 • edited Loading

Overview

Details

Example

References

Blocked by

ZibingZhang commented Dec 6, 2022

odashi commented Dec 7, 2022 • edited Loading

odashi commented Dec 7, 2022

ZibingZhang commented Dec 7, 2022 • edited Loading

odashi commented Dec 7, 2022 • edited Loading

ZibingZhang commented Dec 7, 2022 • edited Loading

odashi commented Dec 7, 2022 • edited Loading

odashi commented Dec 7, 2022 • edited Loading

ZibingZhang commented Dec 7, 2022

ZibingZhang commented Dec 7, 2022

odashi commented Dec 8, 2022 • edited Loading

odashi commented Dec 8, 2022

ZibingZhang commented Dec 8, 2022

odashi commented Dec 8, 2022 • edited Loading

odashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZibingZhang commented Dec 6, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

ZibingZhang commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

ZibingZhang commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 7, 2022 •

edited

Loading

odashi commented Dec 8, 2022 •

edited

Loading

odashi commented Dec 8, 2022 •

edited

Loading