Opened 4 years ago

Last modified 21 months ago

#18713 needs_work defect

Allow almost any callable in matrix constructor

Reported by: rws Owned by:
Priority: major Milestone: sage-8.0
Component: linear algebra Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: u/rws/18713-1 (Commits) Commit: a4340fbaa9a70fed68fca6323c2a03f4617995e4
Dependencies: Stopgaps:

Description

From http://ask.sagemath.org/question/27111/why-can-matrix-not-use-cached-functions/

sage: @cached_function
....: def L(n,k):
....:         if n==k: return 1
....:         if k<0 or k>n: return 0
....:         return L(n-1,k-1)+(n+k-1)*L(n-1,k)
....: 
sage: matrix(ZZ, 8, L)
...
ValueError: Invalid matrix constructor.  Type matrix? for help

Change History (25)

comment:1 Changed 4 years ago by rws

  • Branch set to u/rws/cannot_feed_cached_function_to_matrix

comment:2 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 34d5502150d2a5174984a277a165420f22cf7b7d
  • Status changed from new to needs_review

New commits:

34d550218713: cannot feed cached function to Matrix

comment:3 Changed 4 years ago by tscrim

  • 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 follow-up: Changed 4 years ago by 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?

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 special-case 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.

Last edited 4 years ago by nbruin (previous) (diff)

comment:5 Changed 4 years ago by jdemeyer

  • 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
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:6 in reply to: ↑ 4 Changed 4 years ago by rws

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 Functions.

comment:7 Changed 4 years ago by rws

  • Branch changed from u/rws/cannot_feed_cached_function_to_matrix to u/rws/18713

comment:8 Changed 4 years ago by rws

  • Commit changed from 34d5502150d2a5174984a277a165420f22cf7b7d to 22e665d0c7d8acd54db1c051761f1182b20e125d
  • Milestone changed from sage-6.8 to sage-6.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:

22e665d18713: allow almost any callable in matrix constructor

comment:9 Changed 4 years ago by git

  • Commit changed from 22e665d0c7d8acd54db1c051761f1182b20e125d to da8d9d80a2b929d9069b42a7a66ba78989c6664d

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

da8d9d818713: that sageinspect doctest again

comment:10 Changed 3 years ago by rws

  • Status changed from needs_review to needs_work

comment:11 Changed 2 years ago by rws

  • Branch changed from u/rws/18713 to u/rws/18713-1

comment:12 Changed 2 years ago by rws

  • Commit changed from da8d9d80a2b929d9069b42a7a66ba78989c6664d to a1249da72e888e91ea1cf543b3ad09df72ec75ef
  • Milestone changed from sage-6.10 to sage-7.6
  • Status changed from needs_work to needs_review

New commits:

a1249da18713: Allow almost any callable in matrix constructor

comment:13 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Why are you adding an import numpy?

comment:14 Changed 2 years ago by jdemeyer

Why this?

not (isinstance(arg, Expression) and not isinstance(arg, Function))):

comment:15 follow-up: Changed 2 years ago by 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).

comment:16 Changed 2 years ago by jdemeyer

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 2 years ago by jdemeyer

Some minor details:

  1. Start exception messages with lower case: raise ValueError("When passing... -> raise ValueError("when passing...
  1. 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(n-1,k-1)+(n+k-1)*L(n-1,k)   <---------
Last edited 2 years ago by jdemeyer (previous) (diff)

comment:18 Changed 2 years ago by git

  • Commit changed from a1249da72e888e91ea1cf543b3ad09df72ec75ef to a4340fbaa9a70fed68fca6323c2a03f4617995e4

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

4d9a73eMerge branch 'develop' into t/18713/18713-1
a4340fb18713: address reviewer's comments

comment:19 in reply to: ↑ 15 Changed 2 years ago by rws

  • Milestone changed from sage-7.6 to sage-8.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 follow-up: Changed 2 years ago by 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]

I would like a doctest for this.

comment:21 Changed 2 years ago by tscrim

Jeroen or Ralf, what other iterables would not (necessarily) be handled by the current branch?

comment:22 in reply to: ↑ 20 Changed 2 years ago by rws

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 2 years ago by tscrim

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 2 years ago by tscrim

  • 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 21 months ago by rws

Yes, any Expression is also a callable but we don't want to call it. This breaks the whole elaborate logic.

Last edited 21 months ago by rws (previous) (diff)
Note: See TracTickets for help on using tickets.