Skip to content

removing n_evaluation #559

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.

Sign up for GitHub

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

Draft
mmatera wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

removing n_evaluation #559

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2dd8a5f
removing n_evaluation
mmatera Sep 18, 2022
ae889a0
removing assert
mmatera Sep 19, 2022
dedf6a7
fixing a typo in scipy_utils/integrators
mmatera Sep 19, 2022
3fdad82
commenting changes in to_python, for future revision.
mmatera Sep 20, 2022
3fc8e2d
Merge branch 'master' into more_removing_n_evaluation
mmatera Nov 21, 2022
6438ce7
Merge branch 'master' into more_removing_n_evaluation
mmatera Nov 27, 2022
382017f
Merge branch 'master' into more_removing_n_evaluation
mmatera Dec 31, 2022
dac7a92
isort
mmatera Dec 31, 2022
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
8 changes: 4 additions & 4 deletions mathics/algorithm/optimizers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ def find_root_secant(f, x0, x, opts, evaluation) -> (Number, bool):
return x0, False
if not isinstance(f1, Number):
return x0, False
f0 = f0.to_python(n_evaluation=True)
f1 = f1.to_python(n_evaluation=True)
f0 = eval_N(f0, evaluation).to_python()
f1 = eval_N(f1, evaluation).to_python()
count = 0
while count < maxit:
if f0 == f1:
Expand All @@ -232,7 +232,7 @@ def find_root_secant(f, x0, x, opts, evaluation) -> (Number, bool):
)
if not isinstance(f1, Number):
return x0, False
f1 = f1.to_python(n_evaluation=True)
f1 = eval_N(f1, evaluation).to_python()
continue

inv_deltaf = from_python(1.0 / (f1 - f0))
Expand All @@ -248,7 +248,7 @@ def find_root_secant(f, x0, x, opts, evaluation) -> (Number, bool):
)
if not isinstance(f2, Number):
return x0, False
f2 = f2.to_python(n_evaluation=True)
f2 = eval_N(f2, evaluation).to_python()
f1, f0 = f2, f1
x1, x0 = x2, x1
if x1 == x0 or abs(f2) == 0:
Expand Down
4 changes: 2 additions & 2 deletions mathics/builtin/scipy_utils/integrators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
import sys
from mathics.builtin import check_requires_list
from mathics.core.utils import IS_PYPY
from mathics.builtin.base import check_requires_list
from mathics.core.util import IS_PYPY
Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 20, 2022

Choose a reason for hiding this comment

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

Also, I fixed a typo here, which was avoiding that this module gets loaded.

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 20, 2022

Choose a reason for hiding this comment

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

Fixed also in #560.


if IS_PYPY or not check_requires_list(["scipy", "numpy"]):
raise ImportError
Expand Down
25 changes: 17 additions & 8 deletions mathics/core/expression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,23 +1378,32 @@ def to_python(self, *args, **kwargs):
String -> '"..."'
Function -> python function
numbers -> Python number
If kwarg n_evaluation is given, apply N first to the expression.
"""
from mathics.builtin.base import mathics_to_python

n_evaluation = kwargs.get("n_evaluation", None)
assert n_evaluation is None

head = self._head
if head is SymbolFunction:

from mathics.core.convert.function import expression_to_callable_and_args

vars, expr_fn = self.elements
return expression_to_callable_and_args(expr_fn, vars, n_evaluation)
evaluation = kwargs.get("evaluation", None)
if evaluation:
vars, expr_fn = self.elements
return expression_to_callable_and_args(expr_fn, vars, evaluation)

# Backward compatibility
n_evaluation = kwargs.get("n_evaluation", None)
if n_evaluation is not None:
Copy link
Copy Markdown
Member

@rocky rocky Sep 18, 2022

Choose a reason for hiding this comment

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

Line 1386 or thereabouts has an assert that n_evaluation is None. So this isn't needed. n_evaluation, a relatively new feature, is a hack that should not have been added in the first place.

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 19, 2022

Choose a reason for hiding this comment

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

Indeed, this line should be removed. Regarding n_evaluation, it is a feature that I proposed to add before we have the eval_N function, and I think that was a useful intermediate step. In any case, if we still want that Expression(SymbolFunction,...).to_python(n_evaluation=evaluation) still produce a callable Python object, we can not remove the parameter completely.

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 19, 2022

Choose a reason for hiding this comment

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

BTW, if we want that to_python always returns a "native" Python object, one possibility for Expressions that do not have a clear Python equivalent would be to return a callable object, like

    def result_callable():
        res = expr.evaluate(evaluation)
        if res.sameQ(expr):
             return expr
        else:
             expr.to_python()
    return result_callable

Then, if at some point the expression becomes evaluable and produces a Python native object, then the result would be a Python object...

Copy link
Copy Markdown
Member

@rocky rocky Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding n_evaluation, it is a feature that I proposed to add before we have the eval_N function, and I think that was a useful intermediate step.

I'd like to be very clear about this. In my view this was an intermediary step because there was myopic thinking about the code an a lack of a bigger picture about how interpreters normally work.

The entire operation represented as a class is the kind of thing you find in toy interpreters but not performant interpreters. Performant interpreters do not use class methods to implement built-in operations.

For a list of Python built-in operations see https://docs.python.org/3/library/dis.html#opcode-collections . For a list of GNU Emacs operations see https://github.com/rocky/elisp-bytecode . You can find the same thing for Ruby, Java, Javascript, and so on. It is a necessary prerequisite say if you want to have add JIT technology for example (which we are a very long way from being able to contemplate).

I mention to be aware of this so as not do this as much in the future. There has been a rush to just plunge into into new areas where much is not known either in terms of the existing code or of conventional practice. And that sometimes leads to an equal effort to remove it later.

Remind me why would we want to "support" Expression(SymbolFunction,...).to_python(n_evaluation=evaluation)?

Copy link
Copy Markdown
Member

@rocky rocky Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, if we want that to_python always returns a "native" Python object, one possibility for Expressions that do not have a clear Python equivalent would be to return a callable object, like

    def result_callable():
        res = expr.evaluate(evaluation)
        if res.sameQ(expr):
             return expr
        else:
             expr.to_python()
    return result_callable

Then, if at some point the expression becomes evaluable and produces a Python native object, then the result would be a Python object...

This is a possibility but there would be a different method called. As I have said in the past there was this pervasive vaguess which assists a muddling of concepts. to_python() is used for different kinds of situations which haven't been clearly specified. And that is why we have weird or hybrid behavior such as returning a string when conversion fails instead of raising an exception. (And letting some other routine convert this to a string then if that is what it needs to do).

As I have alluded to before, to_python_literal() would be a better name for at least one of the disentangled functions.

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 19, 2022

Choose a reason for hiding this comment

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

Ok - point taken. I think you are correct. That PR was way too big to easily grasp all that is going on. Especially the relevant part here where this interface was changed. (Github says the diff is too large to display)

OK. That was because I was much less aware than now about how to make a readable PR. This is something that I learn in the hard way during this time...

I should say that I now understand a lot about how this code needs to change to get it to the point where it seems like a normal non-toy interpreter. And I think to some degree we all understand some of this.

It is going to be a long haul. I'd ask then to please stay away from the parser and scanner and compile methods for now. Pattern matching needs also going over and that is on the list.

The way symbols are bound to variables is another thing that is going to be a mess.

Evaluation we've really only started to go over. It is nowhere near gone over though.

Yes, I agree with all that. But then, we can take two positions: leave the code as it is, diagram a master plan, then rework all the existing pieces, and then (let's say, in a couple of years) see if we can start improving the functionality.

The other way (the one in which I think I could be useful now) is to fix peripherical parts with (maybe suboptimal) code, and to add tests to reinforce the compatibility with WMA. This is what I was trying to do during the last months.

Copy link
Copy Markdown
Member

@rocky rocky Sep 19, 2022

Choose a reason for hiding this comment

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

Yes, I agree with all that. But then, we can take two positions: leave the code as it is, diagram a master plan,

There has been a rough plan. I was making my way over the rewrite-apply-eval process. I had gotten to the top-level evaluation. That was blocking Formatting and Boxing work you wanted to do. So I stopped that process so that you would have free reign over Form, Boxing, and Format. While you are doing that I am looking to expand the use of ListExpression and customized kinds of expressions. Association is a natural for this as well.

The Boxing and Formatting work quickly spilled into scanning and parsing - rightly or wrongly. I'll handle issues that spill over there.

The other way (the one in which I think I could be useful now) is to fix peripherical parts with (maybe suboptimal) code, and to add tests to reinforce the compatibility with WMA. This is what I was trying to do during the last months.

In the places where we have no choice, okay. But in the areas where I have a clearer idea of what needs to be done, let's wait on that. With respect specifically to Forms, Boxing, and Format, this is a clear area where you know more about this than I can even hope to understand, so focus on that. The plan that you have put together seems good so far. So just continue with that.

As for this PR, let's put back that assert and remove the more complex code that deprecating something that we now know to be ill-defined.

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 20, 2022

Choose a reason for hiding this comment

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

OK. The assert was removed, and I left the code for converting Expression(SymbolFunction, ...) as it was before, except by the name of the parameter (I have removed the "n_"). If a call is done with the "n_evaluation" parameter, a deprecation warning is shown, and then continue almost as it was before, except that instead of using the Expression(SymbolN,...).evaluate(n_evaluation) idiom, a eval_N() call is used.

Copy link
Copy Markdown
Member

@rocky rocky Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please no deprecation warning for something that should not have been added.

This PR I thought (and actually the previous one) were supposed to remove uses, not deprecate them. The "assert" checks that. In a little while the assert can be removed as well. Thanks.

In a little while the assert could be removed as well.

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Sep 20, 2022

Choose a reason for hiding this comment

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

I commented out both the assert and the code that shows the deprecation. I am going to leave this as a draft, and eventually use this discussion (and the changes) when we want to go over this method.

value = Expression(SymbolN, self).evaluate(n_evaluation)
from mathics.core.evaluators import eval_N
import warnings

warnings.warn(
(
"use of expr.to_python(n_evaluation) is deprecated."
"Use instead eval_N(expr, evaluation).to_python()"
),
DeprecationWarning,
)
value = eval_N(self, n_evaluation)
return value.to_python()

if head is SymbolDirectedInfinity and len(self._elements) == 1:
Expand Down