Add More Functions #15

1MLightyears · 2020-08-01T08:46:42Z

Add support for <,>,>=,<=,!=,is; Add support for complex boolean statements; leave interface for changing "\triangleq"

I ran test_io.py and no assert is triggered. But since new functions are added, maybe I should also add test cases there?

statements; leave interface for changing "\triangleq"

1MLightyears · 2020-08-01T09:03:45Z

oops a minor mistake
in core.py, at line 200:

    logic_operator = r' \operatorname{unknown\_comparator} '

should be

    logic_operator = r' \operatorname{unknown\_operator} '

I copied from above and forgot to modify it.
sorry...

odashi

This request looks not completed: please finish the work described in the request description.

odashi · 2020-08-01T11:49:26Z

latexify/core.py

@@ -36,11 +36,15 @@ def generic_visit(self, node):
  def visit_Module(self, node):
    return self.visit(node.body[0])

-  def visit_FunctionDef(self, node):
+  def visit_FunctionDef(self, node, equal:str=r'\triangleq'):


equal is unused from anywhere. You need to add a user interface to control this option externally.
And I also think this kind of option should be a parameter of __init__: see also how the math_symbol option is implemented. All visitor functions need to follow the NodeVisitor's convention, which takes only the node parameter.

for "equal":
the fact is that I haven't decided how to implement this meanwhile keep the current interface work (maybe allowing @with_latex(equal="\equal") or something like that will be a good idea). I add this only because I seldom use "\triangleq", and I feel it necessary to have "\equal" for some occasions.

I will try to make this work in the next commit.

I meant this option could never be used by users: all visit_*** functions are called internally by NodeVisitor with only the node parameter. We have basically no way to pass extra arguments.

yes I made this mistake because I've just started learning ast and am not so familiar with its logic. Will remove it.
But I still feel it necessary to leave interface for users to change the form of equal mark. If latexify aims to work on the most cases, and "keep minimality about extra conventions"-I strongly agree with this-should be made into practice, then latexify should leave the most freedom for users to customize their output.
If you also agree with this, I will try to find my approach to implement this with no original interface changed.

I have suggested to add a similar option like math_symbol. It should work and not affect the behavior of existing usages.

OK. I take that as the final result of discussion and will try to implement it in the next commit.

odashi · 2020-08-01T11:58:15Z

latexify/core.py

+      return lstr + r'\le ' + rstr
+    elif isinstance(node.ops[0], ast.NotEq):
+      return lstr + r'\ne ' + rstr
+    elif isinstance(node.ops[0], ast.Is):


I'm not sure simply printing is is good or not: could you remove is and is not for now? We need some discussion to make consensus.

for "is"&"is not":
I tested several cases-the layout, the view, the font, of "is" and "is not", and found that what I really need may actually be "\in" and "\notin". Because normal papers would almost never have statements like "$if x is int$", but "$if x \in Z$".
An approach is that the user manually define a variable like Z=type('Z',(int,),{}) and then use if x is Z to have the output "$if x is Z$". So maybe change "$is$" and "$is not$" to "$\in$" and "$\notin$" is better.
I cannot decide this alone, so it's great to make some discussion for this.

How is this going?

For the use-cases like x \in Z, the equivalent Python functionality is isinstance(x, int). The Python's is is not a belongs-to (\in) operator: it checks if the two expressions represent the same instance, e.g., 1 is 1 is True. Also the Python's in is provided basically for collection types. If we want to implement the \in operator, I think we need to take two implementations: the Python's in and the pattern match replacement for isinstance. As the latter one may involve some garbages, it may be good to restrict acceptable types (e.g., number types only).

If we provide some representations of is, I think the nearest one is maybe isomorphic (\equiv) though there's some differences between them.

Oh I thought you mean not to change them at present.
OK, I will change the implementation of the ast.Is clause into \equiv and delete the ast.IsNot clause. As for isinstance, that couldn't be processed in visit_Compare so maybe that might be implemented in the future.

odashi · 2020-08-01T12:01:09Z

latexify/core.py

+    '''
+    logic_operator = r' \operatorname{unknown\_comparator} '
+    if isinstance(node.op, ast.Or):
+      logic_operator = r'\\&\mathrm{Or}\space '


LaTeX has logical binary operators \lor and \land.

OMG I even never heard them!
Thank you for teaching me these. I will change them at the next commit.

odashi · 2020-08-01T12:08:14Z

latexify/core.py

+  # The structure is weak, for I seldom use very complex boolean statements and
+  # currently have no idea how to simply implement a result like "x=0,1" instead of
+  # "x=0 Or x=1" as it is now XD
+  def visit_BoolOp(self, node):


This function needs the same strategy to visit_BinOp to deal with parenthesis appropriately (generally, logical operators has priorities: not > and > or)

for "visit_BoolOp"
At the first version of visit_BoolOp, I actually wrote the not parts. But after testing a few cases I found it kind of silly to have output like "$if not x\equal 10$", it's not something would appear in anyone's paper. Instead what we actually want is "$if x\neq 10$", but that needs a new list or dict to detect and find proper negative statements. I will work on that after next commit.
About the parenthesis, it is true that parenthesis needs to be mentioned, but when it comes to LaTeX output it needs to be reader-friendly. One approach is to add brackets, another is to start a new line when indicating priority like:

if x < 1 \lor x > 10\\ \land x>0

for if (x<1 or x>10)and(x>0).
"$\lor$" and "$\land$" seem not so good at such case btw.

As far as the papers I read, most authors tend to use comma to connect statements have AND relationship, because OR relationship is quite rarely seen. ORs are likely to be clearly indicated when needed.
If you have a better output format for this plz reply me, and I will try to implement the brackets for parenthesis in next commit.

I prefer single-line form with \neg, \lor and \land. It keeps the same syntax to Python except appearances of operators, and would eventually not surprise users. I think we have to keep minimality about extra conventions of LaTeX since any styles are basically not a common consensus of us (though there are already some strong assumptions introduced when I wrote this first).

I agree with the keeping minimality about extra conventions. I will try to implement the brackets for parenthesis in single-line form in the next commit.

odashi · 2020-08-01T12:22:34Z

latexify/core.py

+    node.op(ast.And, ast.Or, ast.Not): type of the boolean operator.
+    node.values(list of ast.Compare, ast.BoolOp): a list of boolean sub-clause.
+    '''
+    logic_operator = r' \operatorname{unknown\_comparator} '


Put this line on the else block.

OK. Change that in the next commit.

odashi · 2020-08-01T12:26:08Z

latexify/core.py

    name_str = r'\operatorname{' + str(node.name) + '}'
    arg_strs = [self._parse_math_symbols(str(arg.arg)) for arg in node.args.args]
    body_str = self.visit(node.body[0])
-    return name_str + '(' + ', '.join(arg_strs) + r') \triangleq ' + body_str
+    # CHANGE:(20200801@1MLightyears)


Remove all CHANGE: comments to keep the code simple. This kind of comments are unnecessary since commit messages should provide enough information.

for "CHANGE"
ah... sorry to bring in my personal habit. I used to work without git.
Will be removed in the next commit.

odashi · 2020-08-01T12:28:17Z

latexify/core.py

+  # "x=0 Or x=1" as it is now XD
+  def visit_BoolOp(self, node):
+    '''
+    When node is an instance of ast.BoolOp, it has:


This information is already provided by ast. We need not to provide the same stuff.

Forget to delete one of the ''' comments - will be delete in the next commit, sorry!

odashi · 2020-08-01T12:33:59Z

Yes, tests should be updated, but the test directory is not well organized for now.

ryxcommar · 2020-08-01T23:36:50Z

Yes I agree, tests aren't very well conceptualized and organized right now. I just threw them in because it's good to have some tests and "some tests" > "no tests". But I did not spend a lot of time making sure it was extremely scalable.

I personally would recommend covering new functionality under the current setup as it's still really small and easy to manage, but eventually that file is going to get too disorganized for its own good. So someone with more experience writing tests should definitely come up with a better system than the one I implemented.

As a note, I also used pytest because I know Google uses it a lot in their GCP products; so I figured @odashi would be familiar with it and prefer it. But I never consulted him about that.

odashi · 2020-08-02T01:44:25Z

@1MLightyears You can push another commit(s) to this request to resolve comments.

odashi · 2020-08-02T01:48:11Z

@ryxcommar Any test frameworks are okay. I personally prefer unittest if there's no particular intent, as it is a part of Python distribution.

odashi

Thanks for taking lots of your effort!
As there are some missing stuff you are willing to implement, we may soon have another PRs.

Add support for <,>,>=,<=,!=,is; Add support for complex boolean

bb06d49

statements; leave interface for changing "\triangleq"

Merge remote-tracking branch 'upstream/develop' into develop

d34b0c9

odashi requested changes Aug 1, 2020

View reviewed changes

odashi added the enhancement label Aug 1, 2020

odashi mentioned this pull request Aug 2, 2020

Configs to control the behavior. #16

Closed

1MLightyears added a commit to 1MLightyears/latexify_py that referenced this pull request Aug 3, 2020

make changes mentioned in google#15

9aa3f4d

make changes mentioned in google#15

ffc70f9

1MLightyears force-pushed the develop branch from 9aa3f4d to ffc70f9 Compare August 3, 2020 14:32

change implementation for ast.Is; delete ast.NotIs

d4a9cc5

odashi approved these changes Aug 5, 2020

View reviewed changes

odashi merged commit 059eed9 into google:develop Aug 5, 2020

odashi added the feature label Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add More Functions #15

Add More Functions #15

1MLightyears commented Aug 1, 2020

1MLightyears commented Aug 1, 2020

odashi left a comment

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

1MLightyears Aug 1, 2020

odashi Aug 2, 2020 •

edited

1MLightyears Aug 2, 2020

odashi Aug 2, 2020

1MLightyears Aug 2, 2020

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

odashi Aug 4, 2020 •

edited

1MLightyears Aug 4, 2020

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

1MLightyears Aug 1, 2020

odashi Aug 2, 2020

1MLightyears Aug 2, 2020

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

odashi Aug 1, 2020

1MLightyears Aug 1, 2020

odashi commented Aug 1, 2020

ryxcommar commented Aug 1, 2020

odashi commented Aug 2, 2020

odashi commented Aug 2, 2020

odashi left a comment

Add More Functions #15

Add More Functions #15

Conversation

1MLightyears commented Aug 1, 2020

1MLightyears commented Aug 1, 2020

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

odashi Aug 2, 2020 • edited

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 Aug 4, 2020 • edited

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

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 commented Aug 1, 2020

ryxcommar commented Aug 1, 2020

odashi commented Aug 2, 2020

odashi commented Aug 2, 2020

odashi left a comment

Choose a reason for hiding this comment

odashi Aug 2, 2020 •

edited

odashi Aug 4, 2020 •

edited