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

MatchOr and guard statements #158

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Conversation

erica-w-fu
Copy link
Contributor

MatchOr and MatchAnd and implemented and tested.

Overview

All of these produce the correct latex code when a user enters a MatchOr case or guard statements in a python match statement.

Details

made edits to visit_Match

  • added guard statement case, calls visit_Compare
  • replaced any instances of "subject_name" with the subject name (ie if someone write match x, x will be the subject name)

created a new function visit_MatchOr

  • will add "\lor" between each of the patterns
  • will invoke self.visit on each pattern

We worked with four collaborators
@erica-w-fu
@Lucybean-hi
@juliawgraham
@Yuqi07

References

#93

Blocked by

#146
#147

juliawgraham and others added 17 commits December 4, 2022 15:11
…andling; 3. added more unit tests (with juliawgraham)
…in Match node; 3. added corresponding unit tests

Co-authored-by: Lucybean-hi <[email protected]>
Co-authored-by: juliawgraham <[email protected]>
Co-authored-by: Erica Fu <[email protected]>
Co-authored-by: Yuqi <[email protected]>
Co-authored-by: Erica Fu <[email protected]>
Co-authored-by: Lucybean-hi <[email protected]>
…rds and matchor)

Co-authored-by: Yuqi <[email protected]>
Co-authored-by: Erica Fu <[email protected]>
Co-authored-by: Lucybean-hi <[email protected]>
@odashi
Copy link
Collaborator

odashi commented Dec 9, 2022

Thanks, could you follow the recent changes in main?

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.

Only general comments

src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
@erica-w-fu
Copy link
Contributor Author

Thanks, could you follow the recent changes in main?

Could you clarify what those recent changes are?

@ZibingZhang
Copy link
Contributor

Could you clarify what those recent changes are?

Your branch is behind the main branch. Can be resolved by merging main into this branch and resolving conflicts.

@Lucybean-hi
Copy link
Contributor

We updated functions based on your suggestions, resolved all conflicts with the current main, and fixed all CI errors. Please take a check.

src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
r"\left\{ \begin{array}{ll} "
+ r" \\ ".join(case_latexes)
+ r" \end{array} \right."
)

latex_final = latex.replace("subject_name", subject_latex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird hack, might cause issues if there's an actual subject_name in the $\LaTeX$ code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is something we considered as well and would love to hear suggestions.

Since each of the visit_Match... functions cannot access the subject_latex variable(each of those functions can only have the ast node and pattern as the input), we had to find a workaround. We tried to use python format and % to insert the subject_latex but the use of % and {} in latex made this very difficult. Do you have any suggestions for how to better implement this (or possible a better name than "subject_name" to replace)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah I see the dilemma, bit perplexing lol. Not entirely sure how to solve but I'll give it some thought. Thanks for tackling match statements btw! Really cool feature :)

Copy link
Contributor

@ZibingZhang ZibingZhang Dec 11, 2022

Choose a reason for hiding this comment

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

Not sure what @odashi thinks of this solution, but you could create a stack of _match_subject_stack: list[str] = [], and at the very beginning of visit_Match do something like

subject_latex = self._expression_codegen.visit(node.subject)
self._match_subject_stack.append(subject_latex)

and then right before you return do something like

latex = (
    r"\left\{ \begin{array}{ll} "
    + r" \\ ".join(case_latexes)
    + r" \end{array} \right."
)
        
self._match_subject_stack.pop()
return latex

and you would also have

    def visit_MatchValue(self, node: ast.MatchValue) -> str:
        """Visit a MatchValue node."""
        latex = self._expression_codegen.visit(node.value)
        return self._match_subject_stack[-1] + " = " + latex

A stack is only required if nested match statements is supported, which it seems like it is, although I have no idea if it's valid $\LaTeX$ output? In any case this situation I think necessitates a test for nested match statements (assuming they're supported), in order to test that in visit_MatchValue, the LHS takes the appropriate _subject_latex, i.e. the inner one should take the inner subject while the outer one takes the outer.

edit: _subject_latexes -> _match_subject_stack

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the @ZibingZhang 's solution is desirable at this point.

One minor point is that the private member _subject_latexes sounds weird for every other visitors, more specific name (like _match_subject_stack) would be better.

src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Show resolved Hide resolved
src/latexify/codegen/function_codegen_match_test.py Outdated Show resolved Hide resolved
r"\left\{ \begin{array}{ll} "
+ r" \\ ".join(case_latexes)
+ r" \end{array} \right."
)

latex_final = latex.replace("subject_name", subject_latex)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the @ZibingZhang 's solution is desirable at this point.

One minor point is that the private member _subject_latexes sounds weird for every other visitors, more specific name (like _match_subject_stack) would be better.

src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen.py Outdated Show resolved Hide resolved
src/latexify/codegen/function_codegen_match_test.py Outdated Show resolved Hide resolved
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.

Thanks for your contribution. Since the new change introduced other issues, I will take over this branch to merge it ASAP.

@ZibingZhang
Copy link
Contributor

@odashi if you're busy would you mind if i looked into this for merging? I think a lot of the conflicts were due to my changes

@odashi
Copy link
Collaborator

odashi commented Jan 13, 2023

@ZibingZhang Yeah please!

@odashi odashi mentioned this pull request Oct 14, 2023
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.

6 participants