-
Notifications
You must be signed in to change notification settings - Fork 393
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
Add assign feature #51
Conversation
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.
Could you add several unit tests to cover the feature proposed in this change?
src/latexify/core.py
Outdated
def visit_Assign(self, node, action): | ||
del action | ||
|
||
self.assign_var[node.targets[0].id] = self.visit(node.value) |
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.
I think this doesn't work correctly if the function has nested defs:
def foo(x):
y = 42
def bar(x):
y = 123
return y
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.
At this moment, I guessed that it would be better to raise an error when the parser found nested functions or classes (all things that define an inner namespace).
@@ -135,6 +150,8 @@ def visit_Attribute(self, node, action): # pylint: disable=invalid-name | |||
|
|||
def visit_Name(self, node, action): # pylint: disable=invalid-name | |||
del action | |||
if node.id in self.assign_var.keys(): | |||
return self.assign_var[node.id] |
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.
Some concerns around this:
-
It may cause exponential expansion. An extreme case:
def fn(x): a = x + x b = a + a c = b + b return c
This may return
$x+x+x+x+x+x+x+x$ . -
If the assigned string has some structure, we need to add appropriate parentheses around the string.
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.
To avoid exponential expansion, I suggest to add an option to activate or not the assignment.
For example:
@latexify.with_latex(assign_mode=False)
def h(x):
a = abs(x)
b = math.exp(math.sqrt(x))
return a * b
will return a = \left|{x}\right| \\ b = \exp{\left({\sqrt{x}}\right)} \\ \mathrm{h}(x) \triangleq ab
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.
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.
Looks good. I think reduce_assignments
is good for the option name (assign_mode
boolean option is somewhat confusing to users)
Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>
It seems I break some tests ahah, I am going to correct this |
There are still some problems with parenthesis. To avoid some problems like this function: @with_latex(reduce_assignments=True)
def f(x):
y = 1 + x
return y ** 2 that would returns But there are some cases where it is useless and unsightly, for example: @with_latex(reduce_assignments=True)
def f(x):
y = math.exp(x)
return y ** 2 returns |
I think it is okay. redundant parentheses are general problems on this library and we need some entire solution on this, but it is somewhat off-topic of this pull request. |
Add assign feature (cf #23). When there are assignments, variables and values are stored to be displayed at the end. These lines:
are the same as: