Ticket #4889 (closed defect: fixed)

Opened 20 months ago

Last modified 6 months ago

document matrix.list() better

Reported by: robertwb Owned by: was
Priority: major Milestone: sage-4.3.3
Component: linear algebra Keywords:
Cc: Author(s): William Stein
Report Upstream: N/A Reviewer(s): Jason Grout, Nick Alexander
Merged in: sage-4.3.3.alpha1 Work issues:

Description

list(M) and M.list() returning different lists is inconsistent. As discussed at  http://groups.google.com/group/sage-support/browse_thread/thread/a7d8b439df769e7 we should have M.entries() which replaces M.list() and deprecate the latter.

The behavior of list(M) will remain the same, and consistency with M[i].

Attachments

trac_4889-part1.patch Download (23.9 KB) - added by was 8 months ago.
part 1, which does what is needed in the matrix directory; another part will mop up.
trac_4889-document_instead_of_deprecate.patch Download (2.5 KB) - added by was 8 months ago.
apply this instead of the previous

Change History

Changed 20 months ago by jason

A reason to have m.list() return the entries is that m.dict() returns the entries, but in "sparse" dictionary format.

Changed 19 months ago by was

I was coding, and I realized that I very strongly object to this ticket -- or at least the nebulousness of it -- I can't even write the code i want the way I want since I know that it will just break for sure as soon as somebody closes this ticket :-(. For me it is an incredibly important design pattern when working with matrices to turn the entire matrix into a list of its entries -- do something with them -- then use the resulting list to make another matrix.

This is exactly modeled on Magma's Eltseq command, which turns almost anything in Magma into a linear sequence, and almost anything in Magma can be reconstructed from that sequence.

Anyway, the list method is not just one off thing that doesn't matter -- it's central to matrices. So either wontfix this ticket or do it asap to get the pain over with.

I do worry that changing this is changing things for change sake, and I'm not convinced that is a good idea...

Changed 19 months ago by jason

I have a partial patch for this. I also worry that we are just changing things to change things, though I agree slightly that the new name (M.entries) is better than M.list in that it is more descriptive.

Your first paragraph seems to indicate that it would be much better for list(M) to return a list of entries, rather than a list of rows. Is that correct?

Changed 19 months ago by jason

For what it's worth, for a numpy array A, the iterator over all entries is given by A.flat

Changed 8 months ago by was

  • upstream set to N/A

See  http://www.wstein.org/home/wstein/build/sage-4.3.1.rc0-boxen-x86_64-Linux/4889-part1.out for the output of doctests on part1. Fixing all those (many) issues is all that is left.

Changed 8 months ago by was

See also  http://www.wstein.org/home/wstein/build/sage-4.3.1.rc0-boxen-x86_64-Linux/4889-part1-error_not_warn.out

Though I just spent a substantial amount of time on this ticket, I'm *seriously* considering arguing again that this change should *not* be made. The reason is simply if literally hundreds of files in the Sage distro are so intensely impacted, then lots of external code will be too. And this change simply isn't *that* important. Better could be to just document the list method better, and point out the subtlety.

Changed 8 months ago by was

part 1, which does what is needed in the matrix directory; another part will mop up.

Changed 8 months ago by was

OK, after working on this for hours and hours, and changing a ridiculous amount of little stuff (see attached patch, still called -part1), I even more strongly believe this ".list()" usage is deeply entrenched throughout all of Sage. I refuse to make this deprecation change, since it will certainly introduce subtle issues in SAge, and will likely break a lot of code that isn't in Sage. Instead I'm posting a patch to document the subtlety clearly.

Changed 8 months ago by was

apply this instead of the previous

Changed 8 months ago by was

  • status changed from new to needs_review

Changed 8 months ago by jason

I'll post a patch which makes .entries an alias for list. I think it's a better name, and better enough that it's worth having two names and encouraging people to use M.entries().

Changed 8 months ago by robertwb

I agree with William that this is too big of a change to make, though +1 to encouraging an alias entries as it is more explicit.

Changed 8 months ago by was

Yes, +1 to the alias.

Changed 7 months ago by ncalexan

  • status changed from needs_review to positive_review
  • reviewer set to Jason Grout, Nick Alexander
  • author set to William Stein

I don't really care if the alias goes in, but this looks fine to me.

Changed 7 months ago by mvngu

  • status changed from positive_review to closed
  • resolution set to fixed
  • merged set to sage-4.3.3.alpha1

Changed 6 months ago by jason

  • summary changed from deprecate matrix.list() to document matrix.list() better

Changing the title to reflect what was actually done.

Changed 6 months ago by jason

I've made a new ticket to add the m.entries() alias: #8308.

Note: See TracTickets for help on using tickets.