mathics / Mathics

This repository is for archival. Please see https://github.com/Mathics3/mathics-core
https://mathics.org
Other
2.08k stars 208 forks source link

Replace `_?TypeQ` by `_Type` #1560

Closed TiagoCavalcante closed 2 years ago

TiagoCavalcante commented 2 years ago

It is faster.

TiagoCavalcante commented 2 years ago

Strange, manual test passes. I'll revert what I breaked.

TiagoCavalcante commented 2 years ago

Strange, the make pytest passes locally.

rocky commented 2 years ago

Thanks for undertaking this kind of thing. We need to focus on performance to fix it.

I merged this into a branch of the same name so I could try it.

Here is what I tried and the output I got:

$ python mathics/docpipeline.py -s Complex
Testing section(s): Complex
Testing section: Mathematical Functions / Complex
b'   1 ( 0): TEST Head[2 + 3*I]'
b'   2 ( 0): TEST Complex[1, 2/3]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[1, 2 / 3]
Wanted: 1 + 2 I / 3

b'   3 ( 0): TEST Abs[Complex[3, 4]]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Abs[Complex[3, 4]]
Wanted: 5

b'   4 ( 0): TEST OutputForm[Complex[2.0 ^ 40, 3]]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[1.09951*^12, 3]
Wanted: 1.09951*^12 + 3. I

b'   5 ( 0): TEST InputForm[Complex[2.0 ^ 40, 3]]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[1.099511627776*^12, 3]
Wanted: 1.099511627776*^12 + 3.*I

b'   6 ( 0): TEST -2 / 3 - I'
b'   7 ( 0): TEST Complex[10, 0]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[10, 0]
Wanted: 10

b'   8 ( 0): TEST 0. + I'
b'   9 ( 0): TEST 1 + 0 I'
b'  10 ( 0): TEST Head[%]'
b'  11 ( 0): TEST Complex[0.0, 0.0]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[0., 0.]
Wanted: 0. + 0. I

b'  12 ( 0): TEST 0. I'
b'  13 ( 0): TEST 0. + 0. I'
b'  14 ( 0): TEST 1. + 0. I'
b'  15 ( 0): TEST 0. + 1. I'
b'  16 ( 0): TEST Complex[1, Complex[0, 1]]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[1, Complex[0, 1]]
Wanted: 0

b'  17 ( 0): TEST Complex[1, Complex[1, 0]]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[1, Complex[1, 0]]
Wanted: 1 + I

b'  18 ( 0): TEST Complex[1, Complex[1, 1]]'
result =!=wanted
----------------------------------------------------------------------
Test failed: Complex in Reference of Built-in Symbols / Mathematical Functions
Reference of Built-in Symbols
Result: Complex[1, Complex[1, 1]]
Wanted: I

9 tests failed.

Here is what I gather is the difference between r_?NumberQ, i_?NumberQ and r_Number, i_Number in Complex[]

The former works where the arguments are expressions while the latter expects the argument to be some sort of Atom (not expression) that is a Number. That is why for Complex[1, 2/3] we are getting the same thing rather than the expected 1 + 2 I / 3. Here 2 / 3 is matching _?NumberQ but not _Number.

TiagoCavalcante commented 2 years ago

Thanks for undertaking this kind of thing.

I like doing this, if there is anything else like this please tell me. (evaluate_fast is in my list)

There is what I tried and the output I got:

I'll run the tests like this and use grep. Is there a way to show only fails?

rocky commented 2 years ago

I like doing this, if there is anything else like this please tell me.

I think what is most needed right now is some sort of benchmark suite to set a baseline.

Something like this might be useful:

import timeit
from mathics.session import MathicsSession
from mathics.core.parser import parse, MathicsSingleLineFeeder
session = MathicsSession(add_builtin=True, catch_interrupt=False)

number = 5000
print(f"Number of iterations: {number}")

# TODO read the list from a file.
expressions = ("Re[x]", "1+2")

for str_expression in expressions:
    expr = parse(session.definitions, MathicsSingleLineFeeder(str_expression))
    print(timeit.timeit(lambda: expr.evaluate(session.evaluation), number=number), str_expression, "evaluate")
    # ...
    print("-" * 30)

And then there is this whole mechanism where we save the data and chart how well we do over time. It might be possible to work this into the workflows system.

I thought I'd start doing this but i probably won't be able to get to it quickly.

And there is a lot of open endedness with respect to managing the data over time so we can track progress (or slippage).

(evaluate_fast is in my list)

thanks

mmatera commented 2 years ago

Maybe some specific queries like _?NumberQ and _?AtomQ should be handled inside the pattern-matching routine as special cases (without cross all the way of building and checking the pattern).

rocky commented 2 years ago

Maybe some specific queries like _?NumberQ and _?AtomQ should be handled inside the pattern-matching routine as special cases (without cross all the way of building and checking the pattern).

I am not sure I fully understand, but would like to. Maybe adding some pseudo (or real) code would help.

rocky commented 2 years ago

@TiagoCavalcanteTrindade About the benchmark code given above... I think this and the data or example cases that shows the goodness of this PR should be done in a separate PR.

The reason is that we want to get a baseline of bechmarks/timings before we start improving. So that suggests that has to be done before this is merged into master.

What do you think, though?

mmatera commented 2 years ago

The problem is that NumericQ tries to determine if a given expression represents a number. For example, BesselJ[3,Root[(#1^5-#^2-1)&,1]] returns True for NumericQ and False for NumberQ. On the other hand, NumericQ[BesselJ[n,t]] returns False. So, to evaluate NumericQ, the core has to go through all the sub-expressions, while NumberQ just needs to do a type check.

https://reference.wolfram.com/language/ref/NumberQ.html https://reference.wolfram.com/language/ref/NumericQ.html

rocky commented 2 years ago

The problem is that NumericQ tries to determine if a given expression represents a number. For example, BesselJ[3,Root[(#1^5-#^2-1)&,1]] returns True for NumericQ and False for NumberQ. On the other hand, NumericQ[BesselJ[n,t]] returns False. So, to evaluate NumericQ, the core has to go through all the sub-expressions, while NumberQ just needs to do a type check.

https://reference.wolfram.com/language/ref/NumberQ.html https://reference.wolfram.com/language/ref/NumericQ.html

Good - this is really great information. However being very thick-headed (slang for slow-witted or stupid), can you give me a specific example in the code where we could use this? Many thanks.

rocky commented 2 years ago

I'll run the tests like this and use grep. Is there a way to show only fails?

No, I don't think so, other than the grep you suggest. It might be a good option to add in the future. (Except that the long term picture is we want to use something more conventional like Sphinx doctest and autodoc which is maintained by others who will probably do a better job at it.)

mmatera commented 2 years ago

Sure. For example, suppose you want to implement NIntegrate: Let's compare NIntegrate[f_?NumericQ,{x,a_?NumericQ,b_?NumericQ}]=... with NIntegrate[f_?NumericQ,{x,a_?NumberQ,b_?NumberQ}]=...

You need to check that the first argument is "numeric" because you are only allowed to integrate numeric functions, and functions are not numbers. On the other hand, you have the integration limits. If you write

NIntegrate[NIntegrate[f[x,y],{x,-Sqrt[1-y^2],Sqrt[1-y^2]}],{y,-1,1}]

the version with NumericQ will try to check (and return True) for each value of y, while the version with NumberQ will not be evaluated until y has a numeric value.

Probably there are many other examples in the core, but I do not have in mind right now.

rocky commented 2 years ago

I just looked at how we implement NIntegrate and it is basically:

"NIntegrate[func_, domain__, OptionsPattern[NIntegrate]]"

so right now there is just the one rule to check. And all type checking is done inside the function which has to be faster than doing this via calling evaluate() basically to check the parameter - assuming the function itself isn't calling evaluate() for the same reason. By the way, I am not seeing this kind of check inside the code right now.

I am looking for just one concrete place in the code and a concrete example where we can make use of this idea and that will speed things up over what we have now.

mmatera commented 2 years ago

OK, it was just an example. A more concrete example, that @TiagoCavalcanteTrindade found is in Complex[x_?NumberQ, y_?NumberQ] against the former implementation Complex[x_?NumericQ, y_?NumericQ]

So, if in some part of an expression, you have Complex[Sqrt[x], Bessel[n,y]] this will not be evaluated. However, each time the kernel tries to evaluate it, in one case you use the expensive function, and in the other case the cheaper. In any case, both implementations are also wrong: Suppose Complex[Sqrt[-1],Sqrt[-1]] (Sqrt[-1] can not be the real part of a complex number, because it is not real!)

Update: in WMA, Complex[a_?NumberQ,b_?NumberQ]:=a+I b disregarding if a or b are also complex numbers..., so the @TiagoCavalcanteTrindade is OK.

rocky commented 2 years ago

If you have time and are up to it. would you put in a PR to fix and handle the way you think would be best?

mmatera commented 2 years ago
TiagoCavalcante commented 2 years ago

@mmatera thanks for the clarification.

About NumericQ vs NumberQ: I think some functions use NumericQ but don't accept numeric quantities, only numbers.

TiagoCavalcante commented 2 years ago

The benchmark code:

import timeit
from mathics.session import MathicsSession
from mathics.core.parser import parse, MathicsSingleLineFeeder
session = MathicsSession(add_builtin=True, catch_interrupt=False)

number = 50000
print(f"Number of iterations: {number}")

session.evaluate("f[x_?NumberQ] := x")
session.evaluate("g[x_?NumericQ] := x")

for str_expression in ("f[x]", "g[x]"):
    expr = parse(session.definitions, MathicsSingleLineFeeder(str_expression))
    print(timeit.timeit(lambda: expr.evaluate(session.evaluation), number=number), str_expression, "evaluate")

The result:

Number of iterations: 50000
1.4538028269998904 f[x] evaluate
4.859601469999689 g[x] evaluate
TiagoCavalcante commented 2 years ago

@mmatera NumberQ vs NumericQ:

Positive[x_?NumericQ] := If[x > 0, True, False, False]

I don't think an expression can be compared with a number.

In[1]:= x+2& > 0
Out[1]= (x + 2&) > 0
TiagoCavalcante commented 2 years ago

@mmatera now I understand. Pi isn't a number, but is numeric.