Opened 4 years ago

Last modified 4 years ago

#18481 needs_info enhancement

Allow matrix subdivide to optionally return a copy

Reported by: rbeezer Owned by:
Priority: minor Milestone: sage-6.9
Component: linear algebra Keywords: matrix subdivide copy
Cc: tgagne Merged in:
Authors: Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: u/rbeezer/matrix-copy-on-subdivide (Commits) Commit: e974b54136e531ca6040455a1f5af72dc0a3a234
Dependencies: Stopgaps:

Description

The .subdivide matrix method acts in place. There are situations when a copy, with the subdivisions, is desirable. An optional keyword, copy will provide this option.

Change History (10)

comment:1 Changed 4 years ago by rbeezer

I'd like to have the sage_input() command preserve the subdivisions of matrix. So the output of sage_input(A) could be something like

matrix(QQ, [[1,2,3], [4,5,6]]).subdivide([1], [2], copy=True)

presuming the copy option proposed here was functional. So this ticket is a precursor to the upcoming sage_input ticket. Work on the present ticket is close to finished.

comment:2 Changed 4 years ago by rbeezer

  • Branch set to u/rbeezer/matrix-copy-on-subdivide
  • Commit set to e974b54136e531ca6040455a1f5af72dc0a3a234

New commits:

e974b5418481: optionally copy matrix when adding subdivisions

comment:3 Changed 4 years ago by rbeezer

  • Status changed from new to needs_review

comment:4 Changed 4 years ago by rbeezer

  • Cc tgagne added

comment:5 Changed 4 years ago by rbeezer

Follow-on ticket, with adjustments to the sage_input() command for matrices, is at #18550.

comment:6 Changed 4 years ago by vdelecroix

  • Milestone changed from sage-6.8 to sage-6.9
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_info

Argh. There is always a choice between inplace and copy argument... we should definitely fix the standard for Sage. A very rough search gives

$ grep "^[[:space:]]*def [a-zA-Z0-9_]*(" $(find . -name "*.py") | grep "copy="
./matrix/matrix_space.py:    def __call__(self, entries=None, coerce=True, copy=True, sparse = False):
./matrix/matrix_space.py:    def matrix(self, x=0, coerce=True, copy=True):
./combinat/combinat.py:    def __init__(self, l, copy=True):
./combinat/designs/group_divisible_designs.py:    def __init__(self, points, groups, blocks, G=None, K=None, lambd=1, check=True, copy=True,**kwds):
./combinat/designs/bibd.py:    def __init__(self, points, blocks, K=None, lambd=1, check=True, copy=True,**kwds):
./combinat/designs/bibd.py:    def __init__(self, points, blocks, k=None, lambd=1, check=True, copy=True,**kwds):
./graphs/generic_graph.py:    def networkx_graph(self, copy=True):
./modules/free_module.py:    def _element_constructor_(self, x, coerce=True, copy=True, check=True):
./geometry/newton_polygon.py:    def vertices(self, copy=True):

against

e$ grep "^[[:space:]]*def [a-zA-Z0-9_]*(" $(find . -name "*.py") | grep "inplace="
./combinat/root_system/dynkin_diagram.py:    def relabel(self, relabelling, inplace=False, **kwds):
./combinat/ordered_tree.py:    def normalize(self, inplace=False):
./combinat/ordered_tree.py:    def dendrog_normalize(self, inplace=False):
./combinat/cluster_algebra_quiver/cluster_seed.py:    def mutate(self, sequence, inplace=True):
./combinat/cluster_algebra_quiver/quiver.py:    def principal_extension(self, inplace=False):
./combinat/cluster_algebra_quiver/quiver.py:    def mutate(self, data, inplace=True):
./combinat/designs/incidence_structures.py:    def relabel(self, perm=None, inplace=True):
./combinat/yang_baxter_graph.py:    def relabel_vertices(self, v, relabel_operator, inplace=True):
./combinat/yang_baxter_graph.py:    def relabel_edges(self, edge_dict, inplace=True):
./combinat/yang_baxter_graph.py:    def relabel_vertices(self, v, inplace=True):
./graphs/digraph.py:    def reverse_edge(self, u, v=None, label=None, inplace=True, multiedges=None):
./graphs/digraph.py:    def reverse_edges(self, edges, inplace=True, multiedges=None):
./graphs/generic_graph.py:    def subgraph(self, vertices=None, edges=None, inplace=False,
./graphs/generic_graph.py:    def _subgraph_by_deleting(self, vertices=None, edges=None, inplace=False,
./graphs/generic_graph.py:    def random_subgraph(self, p, inplace=False):
./graphs/generic_graph.py:    def relabel(self, perm=None, inplace=True, return_map=False, check_input = True, complete_partial_function = True):
./graphs/generic_graph.py:def graph_isom_equivalent_non_edge_labeled_graph(g, partition=None, standard_label=None, return_relabeling=False, return_edge_labels=False, inplace=False, ignore_edge_labels=False):
./modular/arithgroup/arithgroup_perm.py:    def relabel(self, inplace=True):

What do you think? Is it worth opening a thread on sage-devel?

Vincent

comment:7 Changed 4 years ago by rbeezer

Dear Vincent,

Thanks for looking at this, and for the excellent grep'ing skills. Sorry I didn't get to this right away.

Well, it looks like inplace wins by frequency. But a discussion on sage-devel might prove interesting. I'm in no hurry. And maybe others can point to the discussion later (or have it go in the developer's guide now).

I'll be at Sage Days next week (arriving a bit late), so that would be a good time to finish this one off if there is a consensus.

Thanks, Rob

comment:8 Changed 4 years ago by vdelecroix

Hi Rob,

As I actually said on sage-devel, I would like to keep the following semantics for inplace/copy:

  • inplace: modify (or not) the object, like l.remove(1) is an inplace modification on a list l;
  • copy: whether the created object will hold the argument or make a copy of them, like matrix(ZZ, data=[1,2,3,4], copy=False).

For the purpose of this ticket, this would be an inplace.

Vincent

comment:9 follow-up: Changed 4 years ago by rbeezer

Dear Vincent,

Back from vacation and now at Sage Days 68 for a few days before classes begin.

What do you think of Volker's suggestion that there could be a .subdivided() method, which would return a changed copy? That sounds like a good solution to me.

Rob

comment:10 in reply to: ↑ 9 Changed 4 years ago by vdelecroix

Replying to rbeezer:

What do you think of Volker's suggestion that there could be a .subdivided() method, which would return a changed copy? That sounds like a good solution to me.

+1

Note: See TracTickets for help on using tickets.