Skip to content
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.

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

sage.groups.generic: Fix incorrect identity testing #37257

Merged
merged 7 commits into from
Mar 25, 2024

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Feb 8, 2024

The bsgs function used to check is_zero, when it should check == identity (which is given). It also allows me to use the method for "ad-hoc classes":

sage: # modulo arithmetic lmao
....: # I'll just carry around the modulus
....: n = int(12)
....: G_id = (int(0), n)
....: G_gen = (int(1), n)
....: def G_op(P, Q):
....:     return (int((P[0] + Q[0]) % P[1]), P[1])
....: def G_inv(P):
....:     return (int(-P[0] % P[1]), P[1])
....: P = (int(5), n)
....: 
....: from sage.groups.generic import discrete_log
....: discrete_log(P, G_gen, operation=None, ord=n, op=G_op, identity=G_id, inverse=G_inv)

Also fixed styling, that's why there are some random edits

Comment on lines 462 to 463
# Should this be replaced with .zero()? With an extra AttributeError handler?
identity = a.parent()(0)
Copy link
Member

Choose a reason for hiding this comment

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

I think .zero() should be faster, but indeed not all additive groups have a .zero() method...

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 took the violent approach of replacing it with .zero() and fixing the errors. I only found a few use cases of sage.groups.generic in the entire Sage, so I fixed those (only required fixing homset::zero)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, looks promising! Just double-checking: The change from .codomain() to .extended_codomain() in src/sage/schemes/generic/homset.py is an essentially unrelated bugfix, right? And this is what causes the point (a : a) in that doctest in src/sage/schemes/projective/projective_morphism.py to be normalized properly?

Copy link
Contributor Author

@grhkm21 grhkm21 Feb 12, 2024

Choose a reason for hiding this comment

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

Yes they're two unrelated fixes (sorry!!), the extended_codomain is to imitate what schemes/projective/projective_homset.py does (and makes more sense). The normalisation is a separate problem.

It is caused by the following stripped code:

sage: F.<a> = GF(4)
....: ProjectiveSpace(ZZ, 1)._point(S(F), [a, a])
....: ProjectiveSpace(F, 1)._point(S(F), [a, a])
(a : a)
(1 : 1)

(This caller difference is introduced by the extended_codomain to codomain change.) This is because the two lines call different constructors

def _point(self, *args, **kwds):
"""
Construct a point.
For internal use only. See :mod:`morphism` for details.
TESTS::
sage: P2.<x,y,z> = ProjectiveSpace(2, GF(3))
sage: point_homset = P2._point_homset(Spec(GF(3)), P2)
sage: P2._point(point_homset, [1,2,3])
(2 : 1 : 0)
"""
return SchemeMorphism_point_projective_ring(*args, **kwds)
callls SchemeMorphism_point_projective_ring.

def _point(self, *args, **kwds):
"""
Construct a point.
For internal use only. See :mod:`morphism` for details.
TESTS::
sage: P2.<x,y,z> = ProjectiveSpace(2, GF(3))
sage: point_homset = P2._point_homset(Spec(GF(3)), P2)
sage: P2._point(point_homset, [1,2,3])
(2 : 1 : 0)
"""
return SchemeMorphism_point_projective_finite_field(*args, **kwds)
calls SchemeMorphism_point_projective_finite_field which allows division.

I think this is a separate problem, where ._point shouldn't care about the caller's type but rather the argument (homset)'s type, but again it's a separate problem. It's a separate problem.

src/sage/groups/generic.py Outdated Show resolved Hide resolved
src/sage/groups/generic.py Outdated Show resolved Hide resolved
Copy link

Documentation preview for this PR (built with commit e9ce5c1; changes) is ready! 🎉

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@vbraun vbraun merged commit 58fbf26 into sagemath:develop Mar 25, 2024
19 of 22 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants