search

gnu-octave / symbolic

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

Port helpers from list-of-lists to flat+shape #1239

Closed cbm755 closed 1 year ago

cbm755 commented 1 year ago

Related to #1236. I don't see a problem with the list-of-lists approach, but there are only 2 routines using it so we can delete some code if we port them to flat + shape.

@alexvong1995 can you port @sym/private/mat_rclist_access.m to _make_2d_sym? And then delete the make_2d_sym function.

cbm755 commented 1 year ago

In any case, I think we should keep the high-level function so that we use the low-level function only when we have to. I think it's quite common for types / data structures / classes to have multiple constructors. WDYT?

Let's discuss here.

  1. We could keep both "flat + shape" and "list-of-lists".
  2. do some polymorphic thing (when shape= kwarg is passed, we expect a flat array).
  3. keep only flat + shape.

I was working toward 3 but I don't feel that strongly about it. I don't think list-of-lists is possible in general b/c of the reasons outlined elsewhere (and even in the help of SymPy's .tolist() operator).

cbm755 commented 1 year ago

How about make_2d_sym_list_of_lists flattens and then calls _make_2d_sym? That seems the easiest way to keep both.

However, I think every caller of make_2d_sym_list_of_lists will need to think very carefully if they handle empties correctly: which is why I think it might not be worth keeping.

cbm755 commented 1 year ago

Merging this for now, above decisions not really resolved but I don't think this hurts