Expression refactor by rocky · Pull Request #173 · Mathics3/mathics-core

rocky · 2022-02-24T07:20:44Z

No description provided.

Remove class fields that mirror instance fields remove evaluate from BaseExpression

evaluate() in Number is covered by evaluate in Atom Remove the check_stopped() test. If needed either another function will be added and/or it will be covered by a higher level.

before removing a number of them.

mmatera · 2022-02-24T11:35:03Z

    create_expression: Any

+    # __new__ seems to be used because this object references itself.
+    # In particular:


@rocky, Notice that some descendants of BaseExpression decide the class of the object according to the input. For example, Real receives a float or a string and a precision parameter. According to the parameter, a MachineReal or a PrecisionReal object is created. In the case of Symbol, we use __new__ to return always the same instance of symbols with the same name.

@mmatera I had considered that, but experiments show that this might not be the case.

Consider this code:

class Foo: def __new__(cls, *args, **kwargs): self = object.__new__(cls) print("XXX", id(self)) return self x = Foo() y = Foo() print(id(x)) print(id(y)) print(x == y)

If you run it you will see that x and y have different memory locations. And that x is not the same as y.

So what leads you to believe that "new" is doing this?

Also, I should say that if you wanted a single memory instance there is a design pattern to do this, but this isn't the way.

@rocky, no: I am talking about Symbol. I just mention this because I found that sometimes problems appear if you redefine __new__ in a subclass with a different pattern of arguments than the default __new__ in the parent class has.

A link to a line(s) of code would be nice.

Also, when there is non-obvious stuff going on, comments in code are a good place to describe this. If that gets too long an involved, we also have a developer's guide.

Github comments in PRs will probably get lost.

Think of coding like doing school homework in certain classes: you don't get credit for getting the right answer, you also need to show your work and calculations.

One other thing. I am drafting a discussion for this whole Expression mess. So let's hold off on detailed discussions until I post that, and we can carry on the discussions there.

Here is my [small] opinion: maybe making every expression have a different memory allocation is good (makes the things "easier" in other places). But Python has exited in the last decades, if they have chosen to do it this way, probably there is a why (not saying that Python makes the best design choices, it doesn't). So we should (I'm not asking anyone to do this, just saying my opinion) do it the Pythonic way.

mmatera · 2022-02-24T12:42:57Z

mathics-core/mathics/core/symbols.py

Line 826 in d5c4ecf

def __new__(cls, name, sympy_dummy=None):

mathics-core/mathics/core/atoms.py

Line 336 in d5c4ecf

def __new__(cls, value, p=None) -> "Real":

I do not have the code that produces the error (was a failed experiment), but probably you will detect it faster than I if it comes up.

rocky · 2022-02-24T13:26:43Z

mathics-core/mathics/core/symbols.py

Line 826 in d5c4ecf

def __new__(cls, name, sympy_dummy=None):

mathics-core/mathics/core/atoms.py

Line 336 in d5c4ecf

def __new__(cls, value, p=None) -> "Real":

I do not have the code that produces the error (was a failed experiment), but probably you will detect it faster than I if it comes up.

Good - thanks. I will document this weird behavior then in this branch and update the comment.

mmatera · 2022-02-24T14:03:07Z

        self._cache = None
        return self

-    def clear_cache(self):


Why is useful to sort the method by alphabetic order?

When we have very long lists of things it helps me to follow things. Suppose I want to list the 8 juices in V8 juice. I find I can do that more easily if I give the items in alphabetic order.

I know one can always search for stuff, but I'll just tend to scroll when things are in alphabetic order and am pleased when it appears where I expect it. (In the process having passed the other names may be a win too since it reminds me of what's there.)

There are a couple of caveats though. Some methods and definitions need to be in a certain order. And so when the default is alphabetic, the break in order suggests that this kind of thing is going on.

Also, when there is small list of methods, ordering by logical group is okay.

Here I agree, that is why all code editors (which I know) sort the files alphabetically.

(This may not be necessary after drying BaseElement, but as we [I] already saw in the past, it is better to do the things step by step)

TiagoCavalcante

My take would be to merge this and continue in other PRs.

This Looks Great For Me.

TiagoCavalcante · 2022-02-24T16:21:53Z



-class _NumpyConstant(_Constant_Common):
+class _NumpyConstant(_Constant_Common, NumericOperators):


Just a thing that came into my mind now: shouldn't _NumpyConstant be a subclass of Number?

I don't know but I will check and let you know...

This *Constant are superclasses of some Builtins. Builtins are not Elements so, I think

They shouldn't be a subclass of Number that are Elements.

In general, subclasses of Builtin shouldn't implement NumericOperators.

@mmatera, I don't understand this enough to give my opinion about the first, but I agree with the second.

@mmatera Ok, it took me a while to understand what you mean, but yes, I believe you are correct and 1edd13d should address this.

@TiagoCavalcante In effect what mmatera is saying is that inside Python you can't write say Pi() + 3 (with or without the parenthesis) like you could do say Integer(5) + 3. This is because Pi acts as a built-in symbol which needs to be evaluated. You could presumably write in some code to do the evaluation, e.g. call the evaluate() function, but the the whole convenience of using infix + is kind of lost.

So in summary, this kind of shorthand just doesn't apply for Mathics built-in variables/symbols like this.

TiagoCavalcante · 2022-02-24T16:24:01Z

        finally:
            evaluation.dec_recursion_depth()

+    def equal2(self, rhs: Any) -> Optional[bool]:


I think this is kind of unnecessary, as we have sameQ, maybe the == part can be moved to there.

As far as I can tell SameQ and Equal are different, both operators are pervasive over all kinds of expression types. And the note that the specific implementation here is not the same as SameQ.

That said, possibly it could be attached in some other class and just be made something that is mandatory to define.

Your thoughts @mmatera ?

SameQ is something that all elements should handle. equal2 I do not think so, since in general require an Evaluation object to be determined.

TiagoCavalcante · 2022-02-24T16:25:37Z

+        return self
+
+    def flatten_pattern_sequence(self, evaluation) -> "BaseExpression":
+        return self


These functions in this class don't smell quite right for me.

I agree, I think it should be moved to Expression. Your thoughts @mmatera ?

Yep, al these flatten_* should be specific for Expression, not for general elements

TiagoCavalcante · 2022-02-24T16:26:50Z

+        return []

-        return item_is_free(self, form, evaluation)
+    def get_attributes(self, definitions):


Here and in other places I think that @attribute should be used.

I sort of agree but this is a pervasive change and not something to be put into this PR now.

I had also considered this for get_head and get_elements.

I have opened #174 to track this.
Feel free to add to that or correct what is there.

TiagoCavalcante · 2022-02-24T16:27:26Z


-    def is_inexact(self) -> bool:
-        return self.get_precision() is not None
+    # Probably, this method shouldn't be here.


I think so, numbers don't have elements.

Noted in #174 for another PR

TiagoCavalcante · 2022-02-24T16:28:08Z

+    # Compatibily with old code. Deprecated, but remove after a little bit.
+    get_leaves = get_elements
+
+    def get_float_value(self, permit_complex=False):


Maybe it also shouldn't be here.

Noted in #174 for another PR.

TiagoCavalcante · 2022-02-24T16:28:41Z

+    def get_int_value(self):
        return None

+    def get_lookup_name(self):


This function also doesn't smell quite right for me.

I am guessing this should be an Expression thing, not something here. @mmatera ?

Probably this is something that you want to keep since you call it over the elements of an expression to determine the upvalue rules.

mmatera · 2022-02-24T16:49:58Z



+# FIXME: move to new module element.py
+class NumericOperators:


I like this, as a first step. However, notice that Plus[3,"a"] or Times[x,Graphics[{}]] are perfectly valid Expression s in WL.

Nevertheless, from the Python side, maybe it also makes sense to define specialized behaviors for expressions like

Integer(1)+Integer(2)

or

Integer(3)+MachineReal(.12)

The issue here is more about what we need to express in the Python code. While I can sort of see and tolerate writing
abs(Integer(-8)) I am not convinced it is a good idea over writing Expression(SymbolAbs, -8).

And I would not be surprised if down the line we come up with a better way of doing such things.

So the question is whether in mathics Python code we would ever want to write Integer(3) + String("a") or the other thing.

My take is that when the need arises we can reconsider this. But I really doubt the right thing will ever be to ever put those operators in the base class.

And right now clarity and simplicity is way more important that being able to handle hypothetical edge cases like the ones described above.

Sure, inside Mathics, you should be able to do that. Inside Python code implementing Mathics, I'd need convincing though.

Nevertheless, from the Python side, maybe it also makes sense to define specialized behaviors for expressions like

Integer(1)+Integer(2)

or

Integer(3)+MachineReal(.12)

Ok. It is not clear that this is broken in the current code. None of the tests show it. If this isn't used right now in the code base let's leave it for later or feel free to add the mixin in the right place.

My point is: in the Python side, usually you do not need to write abs(Integer(-8)) but something like

def apply(expr, evaluation): '''F[expr_]''' .... new_expr = abs(expr)

in a way that element eventually is Integer(-8) or Symbol("Pi") or Expression("F",37) or "Hello world". With this implementation, the last option would raise an exception. But OK, probably you would need to check that expr is something that makes sense to use as an argument of abs.

Nevertheless, from the Python side, maybe it also makes sense to define specialized behaviors for expressions like

Integer(1)+Integer(2)

or

Integer(3)+MachineReal(.12)

Ok. It is not clear that this is broken in the current code. None of the tests show it. If this isn't used right now in the code base let's leave it for later or feel free to add the mixin in the right place.

No, I do not mean that your changes break something. I'm OK with them. What I am talking about is just to consider possible extensions, just to avoid repeating mistakes. I mean, maybe in the future we realize that makes sense to specialize add for integers, in a way that instead of return an expression Expression("Plus",Integer(1),Integer(2)) returns directly Integer(3) (for example).

Yeah, let's revisit this later.

in a way that element eventually is Integer(-8) or Symbol("Pi") or Expression("F",37) or "Hello world". With this implementation, the last option would raise an exception. But OK, probably you would need to check that expr is something that makes sense to use as an argument of abs.

Yes, I'd rather raise an exception here than east and execute on some mystery meat that might come back. (A friend of mine asked a Vietnamese person if they really ate dogs in Viet Nam; the response was "in the North only").

mmatera · 2022-02-24T17:13:02Z



-class Integers(Builtin):
+class Integers(Builtin, NumericOperators):


Again, a Builtin shouldn't be at the same time NumericOperators

mmatera · 2022-02-24T17:14:58Z

If we keep from here just the changes in the core side and revert the changes in builtins, I would merge it now.

mmatera

LGTM

TiagoCavalcante

LGTM

rocky added 5 commits February 24, 2022 00:58

Note what's up with "__new__"

5ef5826

Note what's up with __new__

adb2c0d

Remove class fields that mirror instance fields remove evaluate from BaseExpression

evaluate() tweaks

f561e60

evaluate() in Number is covered by evaluate in Atom Remove the check_stopped() test. If needed either another function will be added and/or it will be covered by a higher level.

Put in alphabetic order Atom methods

46dd53b

Sort BaseExpression methods...

c293105

before removing a number of them.

rocky marked this pull request as draft February 24, 2022 07:21

mmatera reviewed Feb 24, 2022

View reviewed changes

rocky added 2 commits February 24, 2022 09:25

Document the need for __new__ in more places

d08e251

Split off numeric operators into its own class

cb2a560

TiagoCavalcante approved these changes Feb 24, 2022

View reviewed changes

mmatera reviewed Feb 24, 2022

View reviewed changes

Remove NumberOperation where inappropriate

078b0bb

rocky force-pushed the expression-refactor branch from 1edd13d to 078b0bb Compare February 24, 2022 17:32

rocky marked this pull request as ready for review February 24, 2022 17:38

rocky requested review from TiagoCavalcante and mmatera February 24, 2022 17:38

mmatera approved these changes Feb 24, 2022

View reviewed changes

TiagoCavalcante approved these changes Feb 24, 2022

View reviewed changes

rocky merged commit c75cc99 into master Feb 24, 2022

rocky deleted the expression-refactor branch March 12, 2022 21:50



		class _NumpyConstant(_Constant_Common):
		class _NumpyConstant(_Constant_Common, NumericOperators):



		# FIXME: move to new module element.py
		class NumericOperators:



		class Integers(Builtin):
		class Integers(Builtin, NumericOperators):

Uh oh!

Conversation

rocky commented Feb 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmatera commented Feb 24, 2022

Uh oh!

rocky commented Feb 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TiagoCavalcante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rocky Feb 24, 2022 •

edited

Loading

rocky Feb 24, 2022 •

edited

Loading

rocky Feb 24, 2022 •

edited

Loading

rocky Feb 24, 2022 •

edited

Loading

rocky Feb 24, 2022 •

edited

Loading

rocky Feb 24, 2022 •

edited

Loading