Opened 5 years ago

Closed 5 years ago

#20904 closed enhancement (fixed)

Deprecate Matrix.I

Reported by: jsrn Owned by:
Priority: major Milestone: sage-7.4
Component: linear algebra Keywords: property, consistency
Cc: Merged in:
Authors: Johan Rosenkilde Reviewers: Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: a4ae27c (Commits, GitHub, GitLab) Commit: a4ae27ccca0c8e530546c7074b9f1fb98487a339
Dependencies: Stopgaps:

Status badges

Description

Following two long discussions on sage-devel about @property: https://groups.google.com/forum/#!searchin/sage-devel/heavy/sage-devel/tVa8vs_PHH0/60QajUDYBQAJ https://groups.google.com/forum/#!searchin/sage-devel/heavy/sage-devel/b-u-0WqrtcY/SWhdeoO_BgAJ

No consensus was reached, except that mostly everyone agreed that Matrix.I was bad (in particular, there was people both for and against Matrix.T and Matrix.H).

So this ticket aims to deprecate just Matrix.I, so we can take it out completely at some point in the future.

Change History (22)

comment:1 Changed 5 years ago by jsrn

  • Branch set to u/jsrn/20904_deprecate_matrix_I

comment:2 Changed 5 years ago by jsrn

  • Commit set to 4940917b6e552283a64b75c5bd9ea397606feda3
  • Status changed from new to needs_review

I'm setting this for needs_review. Note that I tried to run the doctests but that I'm getting a gazillion errors all over the place, unrelated to this ticket, for some reason, and so it's difficult for me to check whether this ticket causes new failures.


New commits:

c8e00bdInstate deprecation
4940917Disable warnings during the SageObject test _test_not_implemented_methods

comment:3 Changed 5 years ago by git

  • Commit changed from 4940917b6e552283a64b75c5bd9ea397606feda3 to 17ff1a03e638dd4e70bde68b94c3b6848ccd3149

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

17ff1a0Rm line numbers in doc-test

comment:4 Changed 5 years ago by jsrn

Fix a doc-test problem. Still open for review.

comment:5 Changed 5 years ago by jmantysalo

I got no deprecation warning at all from this code.

comment:7 Changed 5 years ago by jmantysalo

  • Reviewers set to Jori Mäntysalo
  • Status changed from needs_review to needs_work

OK. Maybe it works after #20948, but for now I mark this as needs_work. I can review this then. Please also fill in author name.

comment:8 Changed 5 years ago by jmantysalo

This should work, as #20948 has been closet. But installing this will force big recompiling, so I guess this needs rebasing or merging.

comment:9 Changed 5 years ago by git

  • Commit changed from 17ff1a03e638dd4e70bde68b94c3b6848ccd3149 to a21bd31380bff6d339aee9df82afc296ff95b19a

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

a21bd31Merge branch 'u/jsrn/20904_deprecate_matrix_I' of git://trac.sagemath.org/sage into 20904_deprecate

comment:10 Changed 5 years ago by jsrn

Rebased. The warnings work correctly for me now.

comment:11 Changed 5 years ago by jmantysalo

Works. Thanks.

Please add your real name to Authors-field and put this to needs_review so I can mark my positive review.

comment:12 Changed 5 years ago by jsrn

  • Authors set to Johan Rosenkilde
  • Status changed from needs_work to needs_review

Thanks. I always forget...

comment:13 Changed 5 years ago by jmantysalo

  • Status changed from needs_review to positive_review

comment:14 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

See patchbot

comment:15 Changed 5 years ago by jdemeyer

Why the changes to src/sage/structure/sage_object.pyx anyway?

comment:16 Changed 5 years ago by git

  • Commit changed from a21bd31380bff6d339aee9df82afc296ff95b19a to 994c89a0631cb87b4fb2f952279feb208a08bb3e

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

c2c26caMerge branch 'u/jsrn/20904_deprecate_matrix_I' of git://trac.sagemath.org/sage into 20904_
994c89aRestore warnings also when an exception is thrown.

comment:17 follow-up: Changed 5 years ago by jsrn

  • Status changed from needs_work to needs_review

OK, fixed: the bug was due to not properly reinstating warnings in _test_not_implemented_methods if that method threw an AssertionError.

@jdemeyer: the reason is that _test_not_implemented_methods accesses every attribute on an object. A gazillion TESTS-blocks in sage.matrix runs TestSuite on matrices, which will run _test_not_implemented_methods, which will access every attribute, in particular Matrix.I. Since this is a property, accessing it will "call" it, thus triggering the deprecation warning. Since _test_not_implemented_methods is not about warnings, I thought that disabling *all* warnings would be OK for the duration of the test.

A more efficient version of _test_not_implemented_methods shouldn't actually call properties, of course, and then I wouldn't have to modify the warnings filter. Do you have a suggestion for an alternative version that would behave like this?

Best, Johan

comment:18 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by jdemeyer

OK, got it. One detail: I think it is better to use a try/finally to restore the warnings. That is more robust than adding warnings.filters.pop(0) at every exit point of the function.

comment:19 Changed 5 years ago by git

  • Commit changed from 994c89a0631cb87b4fb2f952279feb208a08bb3e to a4ae27ccca0c8e530546c7074b9f1fb98487a339

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

a4ae27cUse try,finally instead of manual stuff

comment:20 in reply to: ↑ 18 Changed 5 years ago by jsrn

Agreed, fixed now.

comment:21 Changed 5 years ago by jmantysalo

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to positive_review

Works. (But too late to 7.3, changed milestone.)

comment:22 Changed 5 years ago by vbraun

  • Branch changed from u/jsrn/20904_deprecate_matrix_I to a4ae27ccca0c8e530546c7074b9f1fb98487a339
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.