Opened 5 years ago
Last modified 3 years ago
#18713 needs_work defect
Allow almost any callable in matrix constructor
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  u/rws/187131 (Commits)  Commit:  a4340fbaa9a70fed68fca6323c2a03f4617995e4 
Dependencies:  Stopgaps: 
Description
From http://ask.sagemath.org/question/27111/whycanmatrixnotusecachedfunctions/
sage: @cached_function ....: def L(n,k): ....: if n==k: return 1 ....: if k<0 or k>n: return 0 ....: return L(n1,k1)+(n+k1)*L(n1,k) ....: sage: matrix(ZZ, 8, L) ... ValueError: Invalid matrix constructor. Type matrix? for help
Change History (25)
comment:1 Changed 5 years ago by
 Branch set to u/rws/cannot_feed_cached_function_to_matrix
comment:2 Changed 5 years ago by
 Commit set to 34d5502150d2a5174984a277a165420f22cf7b7d
 Status changed from new to needs_review
comment:3 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
Trivial doctest failure (via the patchbot):
File "src/sage/misc/sageinspect.py", line 1959, in sage.misc.sageinspect.sage_getsourcelines Failed example: sage_getsourcelines(matrix)[1] Expected: 732 Got: 745
Once fixed, you can set a positive review on my behalf.
comment:4 followup: ↓ 6 Changed 5 years ago by
The proposed fix mends the reported problem, and no more. However, shouldn't there be certain behaviour for the matrix constructor if it gets passed a *callable* object? or is that too broad?
I don't think so, actually. The comment itself says "callable" and then the check is for particular types, rather than checking if the thing is callable.
I would say:
 restructure, giving preference to checking the passed in argument being iterable. Possibly specialcase some iterables like lists and tuples for efficiency if necessary (and if that makes a difference  I doubt it.
 check for a callable *later*. You can check using
callable(...)
.
I don't know what we should do with objects that are both iterable and callable. I suspect that giving preference to their iterable behaviour will be best.
comment:5 Changed 5 years ago by
 Status changed from needs_review to needs_work
Small detail: do not use
# :trac:`10158`
in code.
Also, instead of
sage: @cached_function # :trac:`18713`
you could write
Check that the matrix constructor works with any callable input (see :trac:`18713`):: sage: @cached_function
comment:6 in reply to: ↑ 4 Changed 5 years ago by
Replying to nbruin:
The proposed fix mends the reported problem, and no more. However, shouldn't there be certain behaviour for the matrix constructor if it gets passed a *callable* object? or is that too broad?
It is in that all symbolic expressions are callable, i.e., they return itself when called with no arguments (which IMHO makes sense) and they try substitution when given arguments (which will hopefully soon raise an exception with #14270 but my hope is fading). So, instead of having special cases of callables your solution needs to exclude those symbolic expressions that are not Function
s.
comment:7 Changed 5 years ago by
 Branch changed from u/rws/cannot_feed_cached_function_to_matrix to u/rws/18713
comment:8 Changed 5 years ago by
 Commit changed from 34d5502150d2a5174984a277a165420f22cf7b7d to 22e665d0c7d8acd54db1c051761f1182b20e125d
 Milestone changed from sage6.8 to sage6.10
 Status changed from needs_work to needs_review
 Summary changed from cannot feed cached function to Matrix to Allow almost any callable in matrix constructor
New commits:
22e665d  18713: allow almost any callable in matrix constructor

comment:9 Changed 5 years ago by
 Commit changed from 22e665d0c7d8acd54db1c051761f1182b20e125d to da8d9d80a2b929d9069b42a7a66ba78989c6664d
Branch pushed to git repo; I updated commit sha1. New commits:
da8d9d8  18713: that sageinspect doctest again

comment:10 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:11 Changed 4 years ago by
 Branch changed from u/rws/18713 to u/rws/187131
comment:12 Changed 4 years ago by
 Commit changed from da8d9d80a2b929d9069b42a7a66ba78989c6664d to a1249da72e888e91ea1cf543b3ad09df72ec75ef
 Milestone changed from sage6.10 to sage7.6
 Status changed from needs_work to needs_review
New commits:
a1249da  18713: Allow almost any callable in matrix constructor

comment:13 Changed 4 years ago by
 Status changed from needs_review to needs_work
Why are you adding an import numpy
?
comment:14 Changed 4 years ago by
Why this?
not (isinstance(arg, Expression) and not isinstance(arg, Function))):
comment:15 followup: ↓ 19 Changed 4 years ago by
If you are allowing any kind of callable, why not allowing any kind of iterable too (it's a valid question since you are making changes to the code for iterables).
comment:16 Changed 4 years ago by
I don't understand of_other_type
. Can you try a positive description (i.e. saying what it is
instead of what it is not). Like is_iterable
. And then a minor optimization: in Cython, use cdef bint
for booleans.
Or perhaps you can get rid of if of_other_type
completely and replace it with something like if entries is None
.
comment:17 Changed 4 years ago by
Some minor details:
 Start exception messages with lower case:
raise ValueError("When passing...
>raise ValueError("when passing...
 The indentation looks strange here:
+ except TypeError: + # not iterable < + from sage.symbolic.expression import Expression + from sage.symbolic.function import Function + if (callable(arg) + and not (isinstance(arg, Expression) < + and not isinstance(arg, Function))): <
and here also:
+ The matrix constructor works with any callable input (see :trac:`18713`)::
+
+ sage: @cached_function
+ ....: def L(n,k):
+ ....: if n==k: return 1 <
+ ....: if k<0 or k>n: return 0 <
+ ....: return L(n1,k1)+(n+k1)*L(n1,k) <
comment:18 Changed 4 years ago by
 Commit changed from a1249da72e888e91ea1cf543b3ad09df72ec75ef to a4340fbaa9a70fed68fca6323c2a03f4617995e4
comment:19 in reply to: ↑ 15 Changed 4 years ago by
 Milestone changed from sage7.6 to sage8.0
 Status changed from needs_work to needs_review
Replying to jdemeyer:
If you are allowing any kind of callable, why not allowing any kind of iterable too (it's a valid question since you are making changes to the code for iterables).
Because it's really over my head atm.
comment:20 followup: ↓ 22 Changed 4 years ago by
Did you test this against something that is callable and iterable, such as a parent that is a finite set? It seems like the iterable behavior takes precedent, but it is not clear from the code that this is correct. For example, does this now work:
sage: R = GF(9, 'a') sage: m = matrix(3, 3, R) sage: m [ 0 a a + 1] [2*a + 1 2 2*a] [2*a + 2 a + 2 1]
I would like a doctest for this.
comment:21 Changed 4 years ago by
Jeroen or Ralf, what other iterables would not (necessarily) be handled by the current branch?
comment:22 in reply to: ↑ 20 Changed 4 years ago by
Replying to tscrim:
Did you test this against something that is callable and iterable, such as a parent that is a finite set? It seems like the iterable behavior takes precedent, but it is not clear from the code that this is correct. For example, does this now work:
sage: R = GF(9, 'a') sage: m = matrix(3, 3, R) sage: m [ 0 a a + 1] [2*a + 1 2 2*a] [2*a + 2 a + 2 1]
It does not (same error as with develop). Should it?
comment:23 Changed 4 years ago by
Well, technically, because it is a callable. Yet, it should be treated as an iterator in this case. So since it errors out in the same way, I think it is okay for now.
comment:24 Changed 4 years ago by
 Status changed from needs_review to needs_work
Doctest failures (see patchbot):
sage t long src/sage/matrix/matrix_symbolic_dense.pyx # 4 doctests failed
comment:25 Changed 3 years ago by
Yes, any Expression
is also a callable
but we don't want to call it. This breaks the whole elaborate logic.
New commits:
18713: cannot feed cached function to Matrix