No proper way to recurse in evalf

[oscarbenjamin]

The problem is that in the FiniteSet case evalf recurses to the args like this: https://github.com/sympy/sympy/blob/b3cae120dad022dd83e338df69c45f60dd95efd9/sympy/sets/sets.py#L1886-L1887

Since that calls _evalf the call will go through to _eval_evalf which in the case of an Add means AssocOp._eval_evalf: https://github.com/sympy/sympy/blob/b3cae120dad022dd83e338df69c45f60dd95efd9/sympy/core/operations.py#L354

However calling x.evalf() leads to evalf(x, ...) where the evalf function is the internal function here: https://github.com/sympy/sympy/blob/b3cae120dad022dd83e338df69c45f60dd95efd9/sympy/core/evalf.py#L1321

The dispatches through the table to call evalf_add: https://github.com/sympy/sympy/blob/b3cae120dad022dd83e338df69c45f60dd95efd9/sympy/core/evalf.py#L519

The evalf_add function has a special handler to ensure accuracy in the face of cancellation.

It seems that when recursing down the args in an evalf call the function to use should be evalf(x, ...) (as is done in the internal routines in the evalf module). The alternatives are all problematic:

1. The x.evalf() method takes the wrong kind of precision and is the public method.
2. The x._evalf() method just called x._eval_evalf and handles a None return. Perhaps this method isn't needed at all.
3. The x._eval_evalf method bypasses the table so it won't e.g. lead to evalf_add.

However the evalf(x, ...) function also isn't suitable for recursion from x._eval_evalf because the _eval_evalf method doesn't receive the arguments that should be passed to evalf(x, ...). The _eval_evalf method is clearly the way for non-core classes to implement evalf but there does not seem to be a proper way for such a method to recurse to its args.

This all seems like a bit of a mess to me and indeed usage around the codebase of .evalf, ._evalf and ._eval_evalf is inconsistent.

Originally posted by @oscarbenjamin in https://github.com/sympy/sympy/issues/20379#issuecomment-730563821

[oscarbenjamin]

We need a new method to replace _eval_evalf etc that can actually propagate options for evalf.

[sidhu1012]

The non-core classes call .evalf first and it tries to solve using evalf_table. If evalf_table fails then it falls back to _eval_evalf which is implemented in classes explicitly.

Non-core class use eval_evalf to parse through its args and call evalf on args individually as all args are independent of each other.

One way to solve this issue is to pass options to _eval_evalf i.e _eval_evalf(prec, options) . This way new method won't be needed and old _eval_evalf could be updated easily.

As in the case of FiniteSet it can be -

def _eval_evalf(self, prec, options): return FiniteSet(*[elem.evalf(n=prec_to_dps(prec), options) for elem in self])

This way x.evalf() would have same precision as FiniteSet.evalf() and all options of evalf could be implied too.

And I agree with @oscarbenjamin x._evalf() is of no use and should be removed.

[sidhu1012]

3. The x._eval_evalf method bypasses the table so it won't e.g. lead to evalf_add.

x._eval_evalf method bypasses the table because it calls ._evalf() which doesn't make sense to me because _eval_evalf is calling ._evalf() for it's args , which is an indirect call to _eval_evalf skipping the args.evalf() thus skipping the evalf_table.

[sidhu1012]

Similar changes are required to EvalMixin class also, but these changes would be slightly different because the EvalMixin class has it's option parameter expanded.

https://github.com/sympy/sympy/blob/b3cae120dad022dd83e338df69c45f60dd95efd9/sympy/core/evalf.py#L1384

[oscarbenjamin]

One way to solve this issue is to pass options to _eval_evalf i.e _eval_evalf(prec, options) . This way new method won't be needed and old _eval_evalf could be updated easily.

The problem with this is that user code and downstream libraries have probably implemented _eval_evalf methods that don't take any arguments. That code would break if we suddenly start passing arguments to _eval_evalf.

[sidhu1012]

The problem with this is that user code and downstream libraries have probably implemented _eval_evalf methods that don't take any arguments. That code would break if we suddenly start passing arguments to _eval_evalf.

I don't think could would break because options would be an optional parameter.

[oscarbenjamin]

If a downstream library has an _eval_evalf method that does not accept options as a parameter then it would raise of the evalf routines started passing an options parameter when calling that _eval_evalf routine.

[sidhu1012]

@oscarbenjamin I think I got your point.

Correct me if I'm wrong.

What we need here is a new _eval_evalf() function which would be defined explicitly in subsequent classes and would work same as _eval_evalf but would take all parameters (option) of evalf and also corrects the calling of ._eval and precision cases in entire codebase.

I would give it a try!

[oscarbenjamin]

We need to presume that a user has made a class something like this:

class MyPI(Expr):
    def _eval_evalf(self, prec):
         return pi.evalf()

p = MyPI()
print((2*p).evalf())

A user might have done this because implementing the _eval_evalf method like this is the expected way to make evalf work. That works and the _eval_evalf method is called by the main evalf code. We need to ensure that any change we make will not break that users code. If the evalf code was changed to cal obj._eval_evalf(prec, options) then their code would break.

So we need to create a new _eval_evalf_options method. The code in evalf that calls _eval_evalf should be changed to check for the presence of the _eval_evalf_options function before using _eval_evalf (we should not presume that users will have subclassed EvalfMixin). There should be tests that implementing something like MyPI still works with either the _eval_evalf or _eval_evalf_options methods.

Then within the sympy codebase all _eval_evalf methods should be changed to _eval_evalf_options and they should all recurse using the evalf function from the sympy.core.evalf module (not the .evalf, ._evalf or ._eval_evalf methods).

Finally the docs should be updated to clearly explain that subclassing EvalfMixin and implementing _eval_evalf_options is the expected way to implement evalf functionality and that all recursive calls should use the evalf function. Also the docs should explain the differences between these methods (e.g. different precisions) and that the _evalf and _eval_evalf methods are essentially deprecated. The place where the _eval_evalf method is still called should have a comment linking back to this issue to make sure that we maintain compatibility for that in future.

[sidhu1012]

@oscarbenjamin Got it! Working on it.

[sidhu1012]

Then within the sympy codebase all _eval_evalf methods should be changed to _eval_evalf_options and they should all recurse using the evalf function from the sympy.core.evalf module (not the .evalf, ._evalf or ._eval_evalf methods).

I have been working on it constantly and found out that _eval_evalf_options should not recurse using the evalf function from the sympy.core.evalf module and should recurse on .evalf because .evalf is the evalf function of EvalMixin and it handles some trivial cases of the evalf function from the sympy.core.evalf mainly NotImplementedError.

The evalf function from the sympy.core.evalf is called from .evalf and if evalf fails then .evalf returns self instead of NotImplementedError which is expected in entire codebase.

This problem could be solved, but would make .evalf method irrelevant and code redundant.

The best way to make evalf consistent is to make _eval_evalf_options which recurse on .evalf, which would take care of evalf automatically and precision won't be compromised.

[oscarbenjamin]

We should not use .evalf for recursion because it takes the wrong kind of precision and is an end user function that does a lot of preprocessing.

[asmeurer]

We could make use of the inspect module to check the signature and deprecate _eval_evalf not taking an options parameter. inspect.signature breaks for code not written in Python, but I doubt there is much of that for SymPy subclasses.

[asmeurer]

Also most of the code in evalf.py should really be on the specific classes. The _eval_evalf method should be general enough that Add._eval_evalf should just be what is currently evalf_add.