gnu-octave / symbolic

A Symbolic Package for Octave using SymPy
https://octave.sourceforge.io/symbolic/
GNU General Public License v3.0
158 stars 36 forks source link

tracker issue for non-Expr in matrices #1124

Open cbm755 opened 2 years ago

cbm755 commented 2 years ago

We have various issues stemming from upstream SymPy's decision (sensible decision, that is!) to not allow things that are not expressions into matrices.

Examples of things we do:

I'll link issues / failing tests / etc to this Issue number.

TODO list

related issues

1107, #1084, etc

upstream issues

cbm755 commented 2 years ago

For 3.0.0, we don't need permanent fix (its only deprecated upstream), but of course would be nice...

cbm755 commented 2 years ago

Alternatively if the goal is just printing support then perhaps TableForm can be used.

https://docs.sympy.org/latest/explanation/active-deprecations.html#deprecated-non-expr-in-matrix

@alexvong243f We should investigate whether TableForm would work instead of Array.

alexvong243f commented 2 years ago

From the doc-string of TableForm, the only documented way to extract the original data is to use as_matrix(), which is obviously not what we want. One can do it using the undocumented attribute of _lines though.

Help on class TableForm in module sympy.printing.tableform:

class TableForm(builtins.object)
 |  TableForm(data, **kwarg)
 |
 |  Create a nice table representation of data.
 |
 |  Examples
 |  ========
 |
 |  >>> from sympy import TableForm
 |  >>> t = TableForm([[5, 7], [4, 2], [10, 3]])
 |  >>> print(t)
 |  5  7
 |  4  2
 |  10 3
 |
 |  You can use the SymPy's printing system to produce tables in any
 |  format (ascii, latex, html, ...).
 |
 |  >>> print(t.as_latex())
 |  \begin{tabular}{l l}
 |  $5$ & $7$ \\
 |  $4$ & $2$ \\
 |  $10$ & $3$ \\
 |  \end{tabular}
 |
 |  Methods defined here:
 |
 |  __init__(self, data, **kwarg)
 |      Creates a TableForm.
 |
 |      Parameters:
 |
 |          data ...
 |                          2D data to be put into the table; data can be
 |                          given as a Matrix
 |
 |          headings ...
 |                          gives the labels for rows and columns:
 |
 |                          Can be a single argument that applies to both
 |                          dimensions:
 |
 |                              - None ... no labels
 |                              - "automatic" ... labels are 1, 2, 3, ...
 |
 |                          Can be a list of labels for rows and columns:
 |                          The labels for each dimension can be given
 |                          as None, "automatic", or [l1, l2, ...] e.g.
 |                          ["automatic", None] will number the rows
 |
 |                          [default: None]
 |
 |          alignments ...
 |                          alignment of the columns with:
 |
 |                              - "left" or "<"
 |                              - "center" or "^"
 |                              - "right" or ">"
 |
 |                          When given as a single value, the value is used for
 |                          all columns. The row headings (if given) will be
 |                          right justified unless an explicit alignment is
 |                          given for it and all other columns.
 |
 |                          [default: "left"]
 |
 |          formats ...
 |                          a list of format strings or functions that accept
 |                          3 arguments (entry, row number, col number) and
 |                          return a string for the table entry. (If a function
 |                          returns None then the _print method will be used.)
 |
 |          wipe_zeros ...
 |                          Do not show zeros in the table.
 |
 |                          [default: True]
 |
 |          pad ...
 |                          the string to use to indicate a missing value (e.g.
 |                          elements that are None or those that are missing
 |                          from the end of a row (i.e. any row that is shorter
 |                          than the rest is assumed to have missing values).
 |                          When None, nothing will be shown for values that
 |                          are missing from the end of a row; values that are
 |                          None, however, will be shown.
 |
 |                          [default: None]
 |
 |      Examples
 |      ========
 |
 |      >>> from sympy import TableForm, Symbol
 |      >>> TableForm([[5, 7], [4, 2], [10, 3]])
 |      5  7
 |      4  2
 |      10 3
 |      >>> TableForm([list('.'*i) for i in range(1, 4)], headings='automatic')
 |        | 1 2 3
 |      ---------
 |      1 | .
 |      2 | . .
 |      3 | . . .
 |      >>> TableForm([[Symbol('.'*(j if not i%2 else 1)) for i in range(3)]
 |      ...            for j in range(4)], alignments='rcl')
 |          .
 |        . . .
 |       .. . ..
 |      ... . ...
 |
 |  __repr__(self)
 |      Return repr(self).
 |
 |  __str__(self)
 |      Return str(self).
 |
 |  as_latex(self)
 |
 |  as_matrix(self)
 |      Returns the data of the table in Matrix form.
 |
 |      Examples
 |      ========
 |
 |      >>> from sympy import TableForm
 |      >>> t = TableForm([[5, 7], [4, 2], [10, 3]], headings='automatic')
 |      >>> t
 |        | 1  2
 |      --------
 |      1 | 5  7
 |      2 | 4  2
 |      3 | 10 3
 |      >>> t.as_matrix()
 |      Matrix([
 |      [ 5, 7],
 |      [ 4, 2],
 |      [10, 3]])
 |
 |  as_str(self)
 |
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
 |
 |  __dict__
 |      dictionary for instance variables (if defined)
 |
 |  __weakref__
 |      list of weak references to the object (if defined)
alexvong243f commented 2 years ago

Is putting non-Expr in Array officially supported? If not, using an undocumented attribute could be a less bad option going forward...

alexvong243f commented 2 years ago

In any case, I think our approach should be general enough such that this implementation detail doesn't matter. There should be minimal code change regardless of which concrete type we decide to use. It's possible that sympy will introduce a new type in the future that we will want to switch to.

Concretely, I think we should change make_matrix_or_array to be something more general. I would suggest changing its name to make_2d_sym. WDYT? It should take an iterable of iterables as inputs instead of only supporting a list of lists.

Also, we may want to create some more helper functions such as is_2d_sym and is_non_matrix_2d_sym to abstract over the implementation details. WDYT?

cbm755 commented 2 years ago

Those ideas sound good to me. New names sound fine.

Uncertain about lack of indexing in tableForm but as you say should be easy to refactor later.

cbm755 commented 2 years ago

re: TableForm versus Array. I'm less certain about the lack indexing: I've just filed #1223 and #1224 which are examples of using linear indexing into Matrix needing adjustment b/c linear indexing does not work with Array (filed https://github.com/sympy/sympy/issues/24007). That problem would get even worse if we need to abstract all indexing for TableForm.

cbm755 commented 2 years ago

list of methods that have failing tests

(feel free to edit!)

alexvong243f commented 2 years ago

------- Original Message ------- On Friday, September 2nd, 2022 at 8:52 PM, Colin B. Macdonald @.***> wrote:

re: TableForm versus Array. I'm less certain about the lack indexing: I've just filed #1223 and #1224 which are examples of using linear indexing into Matrix needing adjustment b/c linear indexing does not work with Array (filed sympy/sympy#24007). That problem would get even worse if we need to abstract all indexing for TableForm.

We can abstract away indexing / referencing 2D sym using a python function, say "ref_2d_sym(A, i, j)", if we want. The syntax is less nice w/o the "A[i][j]" syntactic sugar but it should work.

Right now, the code assumes the use of Array in a lot of places, such as checking "isinstance(x, NDimArray)" explicitly outside of python header. In theory, to abstract away implementation details of 2D sym, one should replace all 2D sym construction, checking, referencing logic with "make_2d_sym(...)", "is_2d_sym(...)", "ref_2d_sym(...)" and so on...

But maybe getting tests pass is more important for now (than worrying about generality).

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

cbm755 commented 2 years ago

But maybe getting tests pass is more important for now (than worrying about generality).

Yes I think so.

alexvong243f commented 2 years ago

------- Original Message ------- On Tuesday, October 11th, 2022 at 5:11 PM, Colin B. Macdonald @.***> wrote:

But maybe getting tests pass is more important for now (than worrying about generality).

Yes I think so.

My reply was sent last month. For some reason, there's a > 1 month delay...

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

cbm755 commented 2 years ago

Yeah its strange, I got a bunch of notifications last night. Some maybe even older (like months ago).

cbm755 commented 2 years ago

And not just from you @alexvong243f. @chrisjohgorman: for some reason we are getting delayed replies; no action required I guess (?)