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

Implement summation with limits using comprehension #30 #32

Merged
merged 9 commits into from
Oct 4, 2022

Conversation

ryosuketc
Copy link
Contributor

This supports summation with limits (super/subscripts) written with comprehension with for loop and range function.
Example usage is outlined in tests/test_io.py
Note:

This supports summation with limits (super/subscripts) written with comprehension with for loop and range function.
@ryosuketc
Copy link
Contributor Author

ryosuketc commented Oct 10, 2020

Support for summation is pending and will be addressed after the following change.
As discussed offline with @odashi , we plan to revamp the implementation of NodeVisitor to enrich information passed between nodes.

latexify/core.py Outdated
@@ -224,6 +244,27 @@ def visit_If(self, node, action): # pylint: disable=invalid-name
latex += self.visit(node)
return latex + r', & \mathrm{otherwise} \end{array} \right.'

def visit_GeneratorExp_sum(self, node): # pylint: disable=invalid-name
Copy link
Contributor Author

@ryosuketc ryosuketc Oct 18, 2020

Choose a reason for hiding this comment

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

This could have been implemented with generic visit_GeneratorExp, which might be useful for FRs in the future (ditto for visit_comprehension). In that case, we might need some additional checks in decorated_lstr_and_arg.

You could view the generic implementation in this commit (sorry it's a bit dirty before refactoring).
0c5d67f

Copy link
Collaborator

Choose a reason for hiding this comment

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

What are FRs?

This function can be reused with other operators (e.g., products and integrals have the same syntax). We can use some general action name rather than sum.

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 bare visitor (=visitor with None action) of every node should return only a LaTeX string. Otherwise every parent node is required to know what kind of objects are returned from children and how to process them to construct LaTeX strings.

Copy link
Contributor Author

@ryosuketc ryosuketc Oct 19, 2020

Choose a reason for hiding this comment

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

What are FRs?

Sorry for not being clear, but I meant feature requests. I thought some operations could use GeneratorExp and there might not be a reason to limit my function only to summation.

However, your point on bare visitors makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function can be reused with other operators (e.g., products and integrals have the same syntax). We can use some general action name rather than sum.

set_bounds as suggested in the new commit?

latexify/core.py Outdated
@@ -224,6 +244,27 @@ def visit_If(self, node, action): # pylint: disable=invalid-name
latex += self.visit(node)
return latex + r', & \mathrm{otherwise} \end{array} \right.'

def visit_GeneratorExp_sum(self, node): # pylint: disable=invalid-name
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are FRs?

This function can be reused with other operators (e.g., products and integrals have the same syntax). We can use some general action name rather than sum.

latexify/core.py Outdated
@@ -224,6 +244,27 @@ def visit_If(self, node, action): # pylint: disable=invalid-name
latex += self.visit(node)
return latex + r', & \mathrm{otherwise} \end{array} \right.'

def visit_GeneratorExp_sum(self, node): # pylint: disable=invalid-name
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 bare visitor (=visitor with None action) of every node should return only a LaTeX string. Otherwise every parent node is required to know what kind of objects are returned from children and how to process them to construct LaTeX strings.

…ts (or bounds)

-   Use n - 1 instead of n in test (the second arg in range is exclusive)
-   Use more general name to support limits operation (sum is not the only operation that could use limits but integral of products are, too)
from typing import NamedTuple


class Actions(NamedTuple):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to use NamedTuple here because of two benefits.

  • It allows dot access - e.g. constants.actions.SET_BOUNDS (vs. constants.actions[key] with a dict implementation)
  • Tuples are immutable.
  • NamedTuple can be written more simpler than a class.

I could have prepared another file actions.py to achieve the same interface, but actions are essentially constants. Thus I preferred to keep it in constants.py.

If you think otherwise, please let me know.

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.

I think this change works, thanks!

@odashi odashi merged commit 37942ea into google:develop Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants