Skip to content
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.

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

SR.var('x', domain='real') and assume(x, 'real') are very slow #30065

Closed
mkoeppe opened this issue Jul 4, 2020 · 26 comments
Closed

SR.var('x', domain='real') and assume(x, 'real') are very slow #30065

mkoeppe opened this issue Jul 4, 2020 · 26 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

(from #30061)

See also:

CC: @egourgoulhon @sagetrac-tmonteil @orlitzky

Component: symbolics

Author: Matthias Koeppe

Branch/Commit: 1fbfb68

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/30065

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jul 4, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

comment:2
sage: %prun for i in range(10000): x = SR.var('x', domain='real')
   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
  1030001   79.176    0.000   87.914    0.000 maxima_lib.py:412(_eval_line)
   500000    5.826    0.000    6.854    0.000 maxima_lib.py:274(max_to_string)
   230000    1.408    0.000   11.558    0.000 interface.py:1146(_repr_)
   500000    1.027    0.000    1.027    0.000 {method 'python' of 'sage.libs.ecl.EclObject' objects}
   250000    0.944    0.000   14.659    0.000 maxima_abstract.py:1610(__len__)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

comment:3
sage: %prun for i in range(3000): x = SR.var('x', domain='real')
   309000   23.496    0.000   26.177    0.000 maxima_lib.py:412(_eval_line)
sage: %prun for i in range(3000): assume(x, 'real')
   309000   29.508    0.000   32.219    0.000 maxima_lib.py:412(_eval_line)

Gets slower every time.

sage: %prun for i in range(3000): assume(x, 'real')
   309000   39.085    0.000   42.002    0.000 maxima_lib.py:412(_eval_line)
sage: %prun for i in range(3000): assume(x, 'real')
   309000   54.613    0.000   57.968    0.000 maxima_lib.py:412(_eval_line)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

Branch: u/mkoeppe/faster_maxima_assumptions

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

Commit: 134da39

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

comment:5

Some low-hanging fruit here.


New commits:

134da39sage.symbolic.assumptions.GenericDeclaration.assume: Validate against cached valid features first

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

comment:6
sage: %prun for i in range(10000): x = SR.var('x', domain='real')
   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    70097    8.210    0.000    8.683    0.000 maxima_lib.py:412(_eval_line)
       25    0.651    0.026    0.651    0.026 {built-in method sage.libs.ecl.ecl_eval}
    10000    0.298    0.000   12.105    0.001 {method 'var' of 'sage.symbolic.ring.SymbolicRing' objects}
    20048    0.273    0.000    0.325    0.000 maxima_lib.py:274(max_to_string)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2020

comment:7

More can be done by caching GenericDeclaration:

diff --git a/src/sage/symbolic/assumptions.py b/src/sage/symbolic/assumptions.py
index 3e0ca736ba..827ab61b67 100644
--- a/src/sage/symbolic/assumptions.py
+++ b/src/sage/symbolic/assumptions.py
@@ -71,11 +71,13 @@ Assumptions are added and in some cases checked for consistency::
 from sage.structure.sage_object import SageObject
 from sage.rings.all import ZZ, QQ, RR, CC
 from sage.symbolic.ring import is_SymbolicVariable
+from sage.structure.unique_representation import UniqueRepresentation
+
 _assumptions = []
 
 _valid_feature_strings = set()
 
-class GenericDeclaration(SageObject):
+class GenericDeclaration(UniqueRepresentation):
     """
     This class represents generic assumptions, such as a variable being
     an integer or a function being increasing. It passes such

... but the semantics of the assume and forget methods are not very clear...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2020

comment:9

I'll investigate further improvements on follow-up tickets. Needs review

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2020

comment:10

Follow-up: #30074

@tscrim
Copy link
Collaborator

tscrim commented Jul 6, 2020

comment:11

Perhaps it would be better to do

-_valid_feature_strings = set(repr(x).strip() for x in list(maxima("features")))
+_valid_feature_strings.update(repr(x).strip() for x in list(maxima("features")))

so it doesn't overwrite the _valid_feature_strings but instead just adds to it?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

1fbfb68Update instead of overwriting

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

Changed commit from 134da39 to 1fbfb68

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2020

comment:13

Good idea, done.

@tscrim
Copy link
Collaborator

tscrim commented Jul 7, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 7, 2020

comment:14

Thanks.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 7, 2020

comment:15

Thank you!

@mjungmath
Copy link

comment:16

Patchbot is complaining. Apparently, this is due to the preexisting code (line 971):

https://www.flake8rules.com/rules/E701.html

Should this be fixed right on the fly?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2020

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

999852dsrc/sage/symbolic/assumptions.py: Fix style warning

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2020

Changed commit from 1fbfb68 to 999852d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2020

Changed commit from 999852d to 1fbfb68

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 14, 2020

comment:19

Actually no, this ticket is already on Volker's branch.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 14, 2020

comment:20

I've put the commit on #30074 instead

@vbraun
Copy link
Member

vbraun commented Jul 15, 2020

Changed branch from u/mkoeppe/faster_maxima_assumptions to 1fbfb68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants