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

ideals of non-maximal orders in number fields #35762

Merged
merged 11 commits into from
Jan 22, 2024

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Jun 13, 2023

Resolves #34198.

@yyyyx4 yyyyx4 force-pushed the public/ideals_for_non_maximal_orders branch from 346b1ad to b122322 Compare December 26, 2023 19:51
@yyyyx4 yyyyx4 marked this pull request as ready for review December 26, 2023 19:56
@yyyyx4 yyyyx4 force-pushed the public/ideals_for_non_maximal_orders branch from 9154a98 to a35bd10 Compare December 30, 2023 13:21
@yyyyx4 yyyyx4 requested a review from pjbruin January 1, 2024 12:58
@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 1, 2024

I know of some follow-up work under active development that depends on this pull request; hence, it would be very helpful if this could get reviewed quickly in order to minimize the amount of effort spent on rebasing those other changes.

@yyyyx4 yyyyx4 requested review from kwankyu and tscrim January 4, 2024 11:53
@pjbruin
Copy link
Contributor

pjbruin commented Jan 8, 2024

I know of some follow-up work under active development that depends on this pull request; hence, it would be very helpful if this could get reviewed quickly in order to minimize the amount of effort spent on rebasing those other changes.

I'm happy to review this. I have one question for now: in doctest changes such as

--- a/src/sage/rings/number_field/class_group.py
+++ b/src/sage/rings/number_field/class_group.py
@@ -32,13 +32,11 @@ EXAMPLES::
     sage: I * I.ideal()   # ideal classes coerce to their representative ideal
     Fractional ideal (4, 1/2*a + 3/2)
 
-    sage: O = K.OK(); O
-    Maximal Order in Number Field in a with defining polynomial x^2 + 23
-    sage: O*(2, 1/2*a + 1/2)
+    sage: K*(2, 1/2*a + 1/2)
     Fractional ideal (2, 1/2*a + 1/2)
-    sage: (O*(2, 1/2*a + 1/2)).is_principal()
+    sage: (K*(2, 1/2*a + 1/2)).is_principal()
     False
-    sage: (O*(2, 1/2*a + 1/2))^3
+    sage: (K*(2, 1/2*a + 1/2))^3
     Fractional ideal (1/2*a - 3/2)
 """
 

could we change O*(2, 1/2*a + 1/2) to something more descriptive, like O.fractional_ideal(2, 1/2*a + 1/2) or K.fractional_ideal(2, 1/2*a + 1/2), rather than to the less descriptive K*(2, 1/2*a + 1/2)?

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 8, 2024

Thanks! Done.

@pjbruin
Copy link
Contributor

pjbruin commented Jan 15, 2024

I have some proposed further changes that suppress the warnings introduced in sage.rings.finite_rings. They are in a new branch at https://github.com/pjbruin/sage/tree/public/ideals_for_non_maximal_orders . What is the correct way to deal with such "reviewer patches"?

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 15, 2024

Thanks! Presumably the easiest way is for me to simply merge in your branch, which is what I just did. Alternatively, I think you could've made a pull request on my branch associated to this pull request.

SageMath version 10.3.beta5, Release Date: 2024-01-14
…rders' into public/ideals_for_non_maximal_orders
@yyyyx4 yyyyx4 force-pushed the public/ideals_for_non_maximal_orders branch from 855ba38 to 68b4462 Compare January 15, 2024 18:50
Copy link

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

@tscrim
Copy link
Collaborator

tscrim commented Jan 16, 2024

Thanks! Presumably the easiest way is for me to simply merge in your branch, which is what I just did. Alternatively, I think you could've made a pull request on my branch associated to this pull request.

Yes, merging in the specific commit(s) is the best way. There's no need for doing an overly complicated local fork PR IMO. (This is usually done after my first option: Curse at GH's non-collaborative workflow until I feel slightly better.)

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 17, 2024

Thank you for the review!

@vbraun vbraun merged commit a9558bb into sagemath:develop Jan 22, 2024
5 checks passed
@yyyyx4 yyyyx4 deleted the public/ideals_for_non_maximal_orders branch February 14, 2024 02:58
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 27, 2024
sagemathgh-38671: construct order ideals by default for number-field orders
    
This completes the change begun in sagemath#34806 and sagemath#35762: When constructing
ideals of an order, return an ideal of that order, rather than a
fractional ideal of the ambient number field.
    
URL: sagemath#38671
Reported by: Lorenz Panny
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 28, 2024
sagemathgh-38671: construct order ideals by default for number-field orders
    
This completes the change begun in sagemath#34806 and sagemath#35762: When constructing
ideals of an order, return an ideal of that order, rather than a
fractional ideal of the ambient number field.
    
URL: sagemath#38671
Reported by: Lorenz Panny
Reviewer(s): Kwankyu Lee
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.

ideals of non-maximal orders in number fields
5 participants