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

SameQ[] and Expression module tweaks #1079

Merged
merged 5 commits into from
Aug 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ubuntu.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:

jobs:
build:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.12', '3.11', '3.8', '3.9', '3.10']
Expand Down
8 changes: 4 additions & 4 deletions mathics/core/element.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
Here we have the base class and related function for element inside an Expression.
"""


from abc import ABC
from typing import Any, Optional, Tuple

from mathics.core.attributes import A_NO_ATTRIBUTES
Expand Down Expand Up @@ -193,7 +193,7 @@ def __ne__(self, other) -> bool:
) or self.get_sort_key() != other.get_sort_key()


class BaseElement(KeyComparable):
class BaseElement(KeyComparable, ABC):
"""
This is the base class from which all other Expressions are
derived from. If you think of an Expression as tree-like, then a
Expand Down Expand Up @@ -290,7 +290,7 @@ def get_float_value(self, permit_complex=False):
def get_int_value(self):
return None

def get_lookup_name(self):
def get_lookup_name(self) -> str:
"""
Returns symbol name of leftmost head. This method is used
to determine which definition must be asked for rules
Expand Down Expand Up @@ -398,7 +398,7 @@ def is_free(self, form, evaluation) -> bool:
def is_inexact(self) -> bool:
return self.get_precision() is not None

def sameQ(self, rhs) -> bool:
def sameQ(self, rhs: "BaseElement") -> bool:
"""Mathics SameQ"""
return id(self) == id(rhs)

Expand Down
52 changes: 22 additions & 30 deletions mathics/core/expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import time
from bisect import bisect_left
from itertools import chain
from typing import Any, Callable, Iterable, List, Optional, Tuple, Type, Union
from typing import Any, Callable, Iterable, Optional, Tuple, Type, Union

import sympy

Expand Down Expand Up @@ -88,21 +88,17 @@
)


def expression_sameQ(self, other):
def eval_SameQ(self, other):
"""
Iterative implementation of SameQ.
Iterative implementation of SameQ[].

Run a tree transversal comparison between `self` and `other`.
Tree traversal comparison between `self` and `other`.
Return `True` if both tree structures are equal.

This implementation avoids the issue with
the recursion limit in Python 3.12
This non-recursive implementation reduces the Python stack needed
in evaluation. Staring in Python 3.12 there is a limit on the
recursion level.
"""
# TODO: Consider a faster implementation.
Copy link
Member Author

Choose a reason for hiding this comment

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

It is not clear to me this is not fast enough. What is done here is equivalent to tail recursion elimination.

Actually, on my home machine I've never ran into the recursion limit problem. I guess I have more memory. The reason Python 3.12 put compiler/OS limitation to recursion was no doubt some sort of security thing. People aren't going to know (or care about) what is being referred to with "the issue".

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, comparing the time the tests takes in GithubActions, it seems that this is faster than the recursive implementation.

# Would be better to use iterators and yield
# instead of this light stack implementation?
# Or maybe using some tail recursive implementation?
# Other ideas in https://www.geeksforgeeks.org/inorder-tree-traversal-without-recursion/

len_elements = len(self.elements)
if len(other._elements) != len_elements:
Expand Down Expand Up @@ -234,14 +230,6 @@ def union(expressions, evaluation) -> Optional["ExpressionCache"]:
):
return None

# FIXME: this is workaround the current situtation that some
# Atoms, like String, have a cache even though they don't need
# it, by virtue of this getting set up in
# BaseElement.__init__. Removing the self._cache in there the
# causes Boxing to mess up. Untangle this mess.
if expr._cache is None:
return None

Copy link
Member Author

Choose a reason for hiding this comment

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

My linter is telling me correctly that expr can be uninitialized, and indeed it is the loop iterator variable in the preceding "for" loop. Testing that after the loop ends is weird. Removing this doesn't cause any test failure.

symbols = set.union(*[expr._cache.symbols for expr in expressions])

return ExpressionCache(
Expand All @@ -250,12 +238,12 @@ def union(expressions, evaluation) -> Optional["ExpressionCache"]:


class Expression(BaseElement, NumericOperators, EvalMixin):
"""
A Mathics3 M-Expression.
"""A Mathics3 (compound) M-Expression.

A Mathics3 M-Expression is a list where the head is a function designator.
(In the more common S-Expression the head is an a Symbol. In Mathics this can be
an expression that acts as a function.
A Mathics3 M-Expression is a list where the head is a function
designator. (In the more common S-Expression the head is an a
Symbol. In Mathics3, this can be an expression that acts as a
function.

positional Arguments:
- head -- The head of the M-Expression
Expand All @@ -266,10 +254,11 @@ class Expression(BaseElement, NumericOperators, EvalMixin):

Keyword Arguments:
- elements_properties -- properties of the collection of elements

"""

_head: BaseElement
_elements: List[BaseElement]
_elements: Tuple[BaseElement]
_sequences: Any
_cache: Optional[ExpressionCache]
elements_properties: Optional[ElementsProperties]
Expand Down Expand Up @@ -492,7 +481,7 @@ def elements(self, values: Iterable):
self.elements_properties = None

def equal2(self, rhs: Any) -> Optional[bool]:
"""Mathics two-argument Equal (==)
"""Mathics3 two-argument Equal (==)
returns True if self and rhs are identical.
"""
if self.sameQ(rhs):
Expand Down Expand Up @@ -762,6 +751,9 @@ def get_head_name(self):
return self._head.name if isinstance(self._head, Symbol) else ""

def get_lookup_name(self) -> str:
"""
Returns symbol name of leftmost head.
"""
lookup_symbol = self._head
while True:
if isinstance(lookup_symbol, Symbol):
Expand Down Expand Up @@ -1140,7 +1132,7 @@ def rewrite_apply_eval_step(self, evaluation) -> Tuple["Expression", bool]:
# used later, include: HoldFirst / HoldAll / HoldRest / HoldAllComplete.

# Note: self._head can be not just a symbol, but some arbitrary expression.
# This is what makes expressions in Mathics be M-expressions rather than
# This is what makes expressions in Mathics3 be M-expressions rather than
# S-expressions.
head = self._head.evaluate(evaluation)

Expand Down Expand Up @@ -1447,14 +1439,14 @@ def round_to_float(
return None

def sameQ(self, other: BaseElement) -> bool:
"""Mathics SameQ"""
"""Mathics3 SameQ"""
if not isinstance(other, Expression):
return False
if self is other:
return True

# All this stuff maybe should be in mathics.eval.expression
return expression_sameQ(self, other)
return eval_SameQ(self, other)

def sequences(self):
cache = self._cache
Expand Down Expand Up @@ -1558,7 +1550,7 @@ def to_python(self, *args, **kwargs):
# )
return py_obj

# Notice that in this case, `to_python` returns a Mathics Expression object,
# Notice that in this case, `to_python` returns a Mathics3 Expression object,
# instead of a builtin native object.
return self

Expand Down