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

Faster and improved N[] #1580

Closed mmatera closed 2 years ago

mmatera commented 2 years ago

In this PR, several bugs in the implementation of N were corrected. Also, I expect that by using apply_N instead of N.apply_with_prec the numeric evaluation has a boost in the performance. @TiagoCavalcante , could you run the benchmarks over this against master?

TiagoCavalcante commented 2 years ago

image

I have run it 3 times and the results were similar.

Tomorrow I'll benchmark the difference of Expression("N", ...).evalute(evaluation vs apply_N.

I'll also try to fix the font size of the bar chart.

mmatera commented 2 years ago

@TiagoCavalcante, when you can, please benchmark this branch again.

rocky commented 2 years ago

It occurs to me that we can and probably should enhance the pytests to facilitate benchmarking the tests. (While it is true that benchmarking and testing are separate, a simple change here can give some idea).

Basically most of the pytests funnel through a few routines. So that could test an environment setting and accumulate timings if that's set. I will add an issue for this.

Also, everyone please note the next weekend we want to move everything. So to the extent that you can work outside of Mathics core or hold off on PRs that might be long lived or that can wait a week, that would be appreciated.

mmatera commented 2 years ago

@rocky, I am seeing if I can get this merged before you clone the core to Mathics3. To do it, I am trying to get all the benchmarks with at least the same performance that master has.

rocky commented 2 years ago

@rocky, I am seeing if I can get this merged before you clone the core to Mathics3. To do it, I am trying to get all the benchmarks with at least the same performance that master has.

All this work for the same performance? I think we should be looking for wins that are at least 10% and defer those that are not for later.

rocky commented 2 years ago

@rocky, I am seeing if I can get this merged before you clone the core to Mathics3. To do it, I am trying to get all the benchmarks with at least the same performance that master has.

A quick test for this is just to run something like:

time pytest test
mmatera commented 2 years ago

@rocky, I am seeing if I can get this merged before you clone the core to Mathics3. To do it, I am trying to get all the benchmarks with at least the same performance that master has.

All this work for the same performance? I think we should be looking for wins that are at least 10% and defer those that are not for later.

Maybe I expressed myself wrong: What I want to get is an improvement in some benchmark tests without spoiling other benchmark tests. Where I think we can make a difference here is for calls of the form N[Table[BesselJ[n, k*(x+I Pi*y)],{n,1000},{k,0,10,.1},{x,0,10,.1},{y,-1,1,.1}]]

i.e., calls of N over nested expressions.

TiagoCavalcante commented 2 years ago

This is what I got: image image

TiagoCavalcante commented 2 years ago

MyG uses apply_N while MyF use Expression("N", ...).

image

The data:

"MyF": {
    "MyF[x]": [
        1000,
        0.28760961400257656
    ],
    "MyF[1]": [
        1000,
        0.26596153400168987
    ],
    "MyF[1.]": [
        1000,
        0.2594361149967881
    ],
    "MyF[2/3]": [
        1000,
        0.7316746830038028
    ],
    "MyF[1.5]": [
        1000,
        0.26102719100163085
    ],
    "MyF[Pi]": [
        1000,
        0.5522810589973233
    ],
    "MyF[I]": [
        1000,
        0.2850018680037465
    ],
    "MyF[I+Pi]": [
        1000,
        1.8530729679987417
    ],
    "MyF[E]": [
        1000,
        0.33258419900084846
    ]
},
"MyG": {
    "MyG[x]": [
        1000,
        0.1876564940030221
    ],
    "MyG[1]": [
        1000,
        0.17291756499616895
    ],
    "MyG[1.]": [
        1000,
        0.17019443600293016
    ],
    "MyG[2/3]": [
        1000,
        0.61899140800233
    ],
    "MyG[1.5]": [
        1000,
        0.16970351099735126
    ],
    "MyG[Pi]": [
        1000,
        0.4375301319960272
    ],
    "MyG[I]": [
        1000,
        0.1906542279975838
    ],
    "MyG[I+Pi]": [
        1000,
        1.7556017180031631
    ],
    "MyG[E]": [
        1000,
        0.22225739099667408
    ]
}

I think apply_N would benefit from evaluate_fast.

@rocky I believe that this difference worths the time it takes to replace Expression("N", ...) by apply_N. What do you think?

mmatera commented 2 years ago

OK, this last table seems to make more sense than the previous. The problem I face trying to avoid calling evaluate(evaluation) is that some symbols need to pass through pattern matching to get their value (like constants). Is there any progress in defining a prototype for evaluate_fast?

rocky commented 2 years ago

OK, this last table seems to make more sense than the previous. The problem I face trying to avoid calling evaluate(evaluation) is that some symbols need to pass through pattern matching to get their value (like constants). Is there any progress in defining a prototype for evaluate_fast?

https://github.com/mathics/Mathics/tree/eval-refactor is the branch that contains evaluate_fast(). I haven't looked at this for a while and don't intend to until after the mathics-core move.

rocky commented 2 years ago

Also, I should be clear on what evaluate_fast() does. In those places where you know that a function like apply_N() can be used, that is the way to go.

evaluate_fast() was a way to get this behavior without knowing that there is an apply_N() function (or rather a method on a particular builtin class object).

There is another variant that sounds like might be useful or needed. In many situations, what you want to call may change over time, but for the most part will be the same method that was called the last time. For example, those things with Table in them may fall into this type of use: each datatype might be the same (e.g. Complex, Rational, Symbolic) but of course different Table invocations may involve different types of entries.

So here what you'd do is start with an evaluate_fast() kind of method, but add a function signature check based on the map key which is its function signature. If the signature matches we're good. Otherwise Expression(...).evaluate(...) needs be used.

Also note that to use either apply_N or evaluate_fast() it is the caller's responsibility to determine legitimacy that the expression that is getting calculated would never be subject to some other rewrite rule that would be applied, should it have gone through the general Expression(...).evaluate(...) mechanism.

TiagoCavalcante commented 2 years ago

Also note that to use either apply_N or evaluate_fast() it is the caller's responsibility to determine legitimacy that the expression that is getting calculated would never be subject to some other rewrite rule that would be applied, should it have gone through the general Expression(...).evaluate(...) mechanism.

Ok, I'll pay attention to it.

rocky commented 2 years ago

MyG uses apply_N while MyF use Expression("N", ...).

image

The data:

"MyF": {
      "MyF[x]": [
              1000,
              0.28760961400257656
      ],
      "MyF[1]": [
              1000,
              0.26596153400168987
      ],
      "MyF[1.]": [
              1000,
              0.2594361149967881
      ],
      "MyF[2/3]": [
              1000,
              0.7316746830038028
      ],
      "MyF[1.5]": [
              1000,
              0.26102719100163085
      ],
      "MyF[Pi]": [
              1000,
              0.5522810589973233
      ],
      "MyF[I]": [
              1000,
              0.2850018680037465
      ],
      "MyF[I+Pi]": [
              1000,
              1.8530729679987417
      ],
      "MyF[E]": [
              1000,
              0.33258419900084846
      ]
},
"MyG": {
      "MyG[x]": [
              1000,
              0.1876564940030221
      ],
      "MyG[1]": [
              1000,
              0.17291756499616895
      ],
      "MyG[1.]": [
              1000,
              0.17019443600293016
      ],
      "MyG[2/3]": [
              1000,
              0.61899140800233
      ],
      "MyG[1.5]": [
              1000,
              0.16970351099735126
      ],
      "MyG[Pi]": [
              1000,
              0.4375301319960272
      ],
      "MyG[I]": [
              1000,
              0.1906542279975838
      ],
      "MyG[I+Pi]": [
              1000,
              1.7556017180031631
      ],
      "MyG[E]": [
              1000,
              0.22225739099667408
      ]
}

@TiagoCavalcante For this kind of comparison it would be nice change the YAML to add a kind of compare-group. Perhaps something like:

...
categories:
  N: 
    compare-groups:
      symbol: 
        - "MyF[x]"
        - "MyG[x]"

      constant-integer: 
        - "MyF[1]"
        - "MyG[1]"

      constant-real: 
        - "MyF[1.]"
        - "MyG[1.]"

What do you think?

TiagoCavalcante commented 2 years ago

What I thought was an option compare_groups. YAML is easier to reproduce, but this would be hard to implement.

compare_groups: true in the root of the file would be easier to implement. And doesn't makes so much sense it be inside a category, because or the plot is single or a comparsion between groups, or a comparsion between branches.

Your thoughts?

rocky commented 2 years ago

What I thought was an option compare_groups. YAML is easier to reproduce, but this would be hard to implement.

compare_groups: true in the root of the file would be easier to implement. And doesn't makes so much sense it be inside a category, because or the plot is single or a comparsion between groups, or a comparsion between branches.

Your thoughts?

Yes, you are correct - that makes more sense. Make it happen!

mmatera commented 2 years ago

Here we go... imagen

imagen

rocky commented 2 years ago

They were mostly dependence problems with conda, and that I hadn't get how mathics-benchmarks was supposed to work. Now could install and run it, but I am not sure about how to understand the results.

@mmatera Thanks for including the benchmarks!

It literally took me several hours to install it, but then I added/corrected the needed dependencies afterwards (this is a setup.py thing so not everyone understands how that works). But I was hoping that it wouldn't have been as hard to install afterwards.

As for running it, yes, that too took me a while to understand. And the process may still be shifting around as more kinds of use cases come into being. We need to write a README on this, but I haven't had time to think about that let alone do it.