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

Define behaviour of < and > for fractional ideals in a quaternion algebra #37113

Merged

Conversation

syndrakon
Copy link

Previously, comparison of quaternion algebra fractional ideals (in a quaternion algebra over QQ) was documented only for equality and did compare the matrices under HNF of bases of the ideals. This gave strange and undocumented behavior when comparing two ideals with inequalities.

This comparison is now replaced by the comparison of the fractional ideals as free modules.

Now "smaller than" means "included in".

#sd123

…uaternion algebra over QQ

"smaller than" now means "included in"
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

a few comments on the coding style.

@@ -1365,7 +1365,8 @@ def right_ideals(self, B=None):
ideals_theta[J_theta] = [J]
verbose("found %s of %s ideals" % (len(ideals), self.dimension()), level=2)
if len(ideals) >= self.dimension():
ideals = tuple(sorted(ideals))
#order by basis matrix (as ideals were previously ordered) for backward compatibility and deterministic order of the output
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments need a space after # and are usually aligned in 80 columns mode

-                            #order by basis matrix (as ideals were previously ordered) for backward compatibility and deterministic order of the output
+                            # order by basis matrix (as ideals were previously
+	 		     # ordered) for backward compatibility and
+	 		     # deterministic order of the output

True
sage: O >= O
True

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this empty line.

@@ -2393,8 +2393,35 @@ def _richcmp_(self, right, op):

sage: I != I # indirect doctest
False

TESTS:
Copy link
Contributor

Choose a reason for hiding this comment

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

TESTS: -> TESTS::

and add en empty line after TESTS::

@syndrakon
Copy link
Author

Thank you for the explanation, I added your suggestions.

Copy link

github-actions bot commented Feb 1, 2024

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

@syndrakon
Copy link
Author

I am not certain what to to about the failing tests:

The first one failed in a completely unrelated file (src/sage_setup/clean.py) and I cannot reproduce the test failure locally.

The second says that the empty line between examples and tests is not covered by any test. However, I that this line seems necessary since other functions using examples and tests have it also and removing it causes the test parser to fail.

@yyyyx4
Copy link
Member

yyyyx4 commented Feb 2, 2024

Indeed, these look like flukes. (The clean.py failure is in almost every test log at the moment, and complaining about an untested empty line in a docstring is obviously nonsense...)

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 to me, thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 7, 2024
…n a quaternion algebra

    
Previously, comparison of quaternion algebra fractional ideals (in a
quaternion algebra over QQ) was documented only for equality and did
compare the matrices under HNF of bases of the ideals. This gave strange
and undocumented behavior when comparing two ideals with inequalities.

This comparison is now replaced by the comparison of the fractional
ideals as free modules.

Now "smaller than" means "included in".

#sd123
    
URL: sagemath#37113
Reported by: syndrakon
Reviewer(s): David Coudert, Lorenz Panny
@vbraun vbraun merged commit 53c3468 into sagemath:develop Feb 13, 2024
20 of 22 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 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.

None yet

5 participants