Skip to content

Deprecate is_AbelianGroup, is_DualAbelianGroup, is_MatrixGroup #37898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 12, 2024
Merged

Deprecate is_AbelianGroup, is_DualAbelianGroup, is_MatrixGroup #37898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60f85ea
src/sage/groups/abelian_gps/abelian_group.py: Deprecate is_AbelianGroup
mkoeppe Apr 29, 2024
c0b4cd5
src/sage/groups/abelian_gps/dual_abelian_group.py: Deprecate is_DualA…
mkoeppe Apr 29, 2024
ec586f2
src/sage/groups/matrix_gps/matrix_group.py: Deprecate is_MatrixGroup
mkoeppe Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/sage/groups/abelian_gps/abelian_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,10 +458,16 @@ def is_AbelianGroup(x):
sage: F = AbelianGroup(5,[5,5,7,8,9], names=list("abcde")); F
Multiplicative Abelian group isomorphic to C5 x C5 x C7 x C8 x C9
sage: is_AbelianGroup(F)
doctest:warning...
DeprecationWarning: the function is_AbelianGroup is deprecated;
use 'isinstance(..., AbelianGroup_class)' instead
See https://github.com/sagemath/sage/issues/37898 for details.
True
sage: is_AbelianGroup(AbelianGroup(7, [3]*7))
True
"""
from sage.misc.superseded import deprecation
deprecation(37898, "the function is_AbelianGroup is deprecated; use 'isinstance(..., AbelianGroup_class)' instead")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't that be use 'in Groups.Commutative', @fchapoton?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked, yes, that's the pattern. For example:

deprecation(35999, "the function is_CommutativeAlgebra is deprecated; use '... in Algebras(base_ring).Commutative()' instead")

or

deprecation(35253, "the function is_Algebra is deprecated; use '... in Algebras(base_ring)' instead")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did use this pattern in these other deprecation messages when there was at least some indication that these is_... functions were used with this meaning.
But is_AbelianGroup has never been used with a meaning other than this specific isinstance test.

return isinstance(x, AbelianGroup_class)


Expand Down Expand Up @@ -569,7 +575,7 @@ def is_isomorphic(left, right):
sage: G1.is_isomorphic(G2)
True
"""
if not is_AbelianGroup(right):
if not isinstance(right, AbelianGroup_class):
return False
return left.elementary_divisors() == right.elementary_divisors()

Expand Down
6 changes: 6 additions & 0 deletions src/sage/groups/abelian_gps/dual_abelian_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ def is_DualAbelianGroup(x):
sage: F = AbelianGroup(5,[3,5,7,8,9], names=list("abcde"))
sage: Fd = F.dual_group()
sage: is_DualAbelianGroup(Fd)
doctest:warning...
DeprecationWarning: the function is_DualAbelianGroup is deprecated;
use 'isinstance(..., DualAbelianGroup_class)' instead
See https://github.com/sagemath/sage/issues/37898 for details.
True
sage: F = AbelianGroup(3,[1,2,3], names='a')
sage: Fd = F.dual_group()
Expand All @@ -94,6 +98,8 @@ def is_DualAbelianGroup(x):
sage: F.gens()
(1, a1, a2)
"""
from sage.misc.superseded import deprecation
deprecation(37898, "the function is_DualAbelianGroup is deprecated; use 'isinstance(..., DualAbelianGroup_class)' instead")
return isinstance(x, DualAbelianGroup_class)


Expand Down
8 changes: 7 additions & 1 deletion src/sage/groups/matrix_gps/matrix_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ def is_MatrixGroup(x):

sage: from sage.groups.matrix_gps.matrix_group import is_MatrixGroup
sage: is_MatrixGroup(MatrixSpace(QQ, 3))
doctest:warning...
DeprecationWarning: the function is_MatrixGroup is deprecated;
use 'isinstance(..., MatrixGroup_base)' instead
See https://github.com/sagemath/sage/issues/37898 for details.
False
sage: is_MatrixGroup(Mat(QQ, 3))
False
Expand All @@ -86,6 +90,8 @@ def is_MatrixGroup(x):
sage: is_MatrixGroup(MatrixGroup([matrix(2, [1,1,0,1])]))
True
"""
from sage.misc.superseded import deprecation
deprecation(37898, "the function is_MatrixGroup is deprecated; use 'isinstance(..., MatrixGroup_base)' instead")
return isinstance(x, MatrixGroup_base)

###################################################################
Expand Down Expand Up @@ -499,7 +505,7 @@ def __richcmp__(self, other, op):
sage: G != H
False
"""
if not is_MatrixGroup(other):
if not isinstance(other, MatrixGroup_base):
return NotImplemented

if self is other:
Expand Down
11 changes: 3 additions & 8 deletions src/sage/matrix/matrix_space.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
from sage.features import PythonModule
lazy_import('sage.matrix.matrix_gfpn_dense', ['Matrix_gfpn_dense'],
feature=PythonModule('sage.matrix.matrix_gfpn_dense', spkg='meataxe'))
lazy_import('sage.groups.matrix_gps.matrix_group', ['MatrixGroup_base'])

_Rings = Rings()
_Fields = Fields()
Expand Down Expand Up @@ -1392,14 +1393,8 @@ def _coerce_map_from_(self, S):
pass
else:
MS = meth_matrix_space()

try:
from sage.groups.matrix_gps.matrix_group import is_MatrixGroup
except ImportError:
pass
else:
if is_MatrixGroup(S):
return self.has_coerce_map_from(MS)
if isinstance(S, MatrixGroup_base):
return self.has_coerce_map_from(MS)

try:
from sage.modular.arithgroup.arithgroup_generic import is_ArithmeticSubgroup
Expand Down
4 changes: 2 additions & 2 deletions src/sage/modular/arithgroup/congroup_generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,9 @@ def CongruenceSubgroup_constructor(*args):
TypeError: Ring of definition must be Z / NZ for some N
"""
from sage.groups.matrix_gps.finitely_generated import MatrixGroup
from sage.groups.matrix_gps.matrix_group import is_MatrixGroup
from sage.groups.matrix_gps.matrix_group import MatrixGroup_base

if is_MatrixGroup(args[0]):
if isinstance(args[0], MatrixGroup_base):
G = args[0]

elif isinstance(args[0], list):
Expand Down