Opened 5 years ago
Closed 5 years ago
#20904 closed enhancement (fixed)
Deprecate Matrix.I
Reported by:  jsrn  Owned by:  

Priority:  major  Milestone:  sage7.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: 
Description
Following two long discussions on sagedevel about @property
: https://groups.google.com/forum/#!searchin/sagedevel/heavy/sagedevel/tVa8vs_PHH0/60QajUDYBQAJ
https://groups.google.com/forum/#!searchin/sagedevel/heavy/sagedevel/bu0WqrtcY/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
 Branch set to u/jsrn/20904_deprecate_matrix_I
comment:2 Changed 5 years ago by
 Commit set to 4940917b6e552283a64b75c5bd9ea397606feda3
 Status changed from new to needs_review
comment:3 Changed 5 years ago by
 Commit changed from 4940917b6e552283a64b75c5bd9ea397606feda3 to 17ff1a03e638dd4e70bde68b94c3b6848ccd3149
Branch pushed to git repo; I updated commit sha1. New commits:
17ff1a0  Rm line numbers in doctest

comment:4 Changed 5 years ago by
Fix a doctest problem. Still open for review.
comment:5 Changed 5 years ago by
I got no deprecation warning at all from this code.
comment:6 Changed 5 years ago by
Perhaps it is due to this: https://groups.google.com/forum/#!topic/sagedevel/NCuA7F4YOLk
comment:7 Changed 5 years ago by
 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
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
 Commit changed from 17ff1a03e638dd4e70bde68b94c3b6848ccd3149 to a21bd31380bff6d339aee9df82afc296ff95b19a
Branch pushed to git repo; I updated commit sha1. New commits:
a21bd31  Merge branch 'u/jsrn/20904_deprecate_matrix_I' of git://trac.sagemath.org/sage into 20904_deprecate

comment:10 Changed 5 years ago by
Rebased. The warnings work correctly for me now.
comment:11 Changed 5 years ago by
Works. Thanks.
Please add your real name to Authorsfield and put this to needs_review so I can mark my positive review.
comment:12 Changed 5 years ago by
 Status changed from needs_work to needs_review
Thanks. I always forget...
comment:13 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:15 Changed 5 years ago by
Why the changes to src/sage/structure/sage_object.pyx
anyway?
comment:16 Changed 5 years ago by
 Commit changed from a21bd31380bff6d339aee9df82afc296ff95b19a to 994c89a0631cb87b4fb2f952279feb208a08bb3e
comment:17 followup: ↓ 18 Changed 5 years ago by
 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 ; followup: ↓ 20 Changed 5 years ago by
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
 Commit changed from 994c89a0631cb87b4fb2f952279feb208a08bb3e to a4ae27ccca0c8e530546c7074b9f1fb98487a339
Branch pushed to git repo; I updated commit sha1. New commits:
a4ae27c  Use try,finally instead of manual stuff

comment:20 in reply to: ↑ 18 Changed 5 years ago by
Agreed, fixed now.
comment:21 Changed 5 years ago by
 Milestone changed from sage7.3 to sage7.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
 Branch changed from u/jsrn/20904_deprecate_matrix_I to a4ae27ccca0c8e530546c7074b9f1fb98487a339
 Resolution set to fixed
 Status changed from positive_review to closed
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:
Instate deprecation
Disable warnings during the SageObject test _test_not_implemented_methods