Skip to content

add .conductor() and .order_of_conductor() methods for orders in quadratic fields #35271

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

Jump to bottom
Merged
merged 1 commit into from
Apr 13, 2023
Merged

add .conductor() and .order_of_conductor() methods for orders in quadratic fields #35271

merged 1 commit into from
Apr 13, 2023

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Mar 13, 2023

This patch adds two tiny convenience methods:

  • .conductor() for orders in quadratic fields, returning the unique positive integer $f$ such that the discriminant equals $f^2\cdot D$ with $D$ a fundamental discriminant.
  • .order_of_conductor() for quadratic fields, returning the unique order with the given conductor.

tscrim
tscrim requested changes Mar 13, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM modulo the two fairly trivial things.

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 81.81% and project coverage change: -0.01 ⚠️

Comparison is base (f449b14) 88.62% compared to head (107fbfe) 88.62%.

❗ Current head 107fbfe differs from pull request most recent head cdc7f4a. Consider uploading reports for the commit cdc7f4a to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #35271      +/-   ##
===========================================
- Coverage    88.62%   88.62%   -0.01%     
===========================================
  Files         2148     2148              
  Lines       398653   398664      +11     
===========================================
+ Hits        353308   353312       +4     
- Misses       45345    45352       +7
Impacted Files Coverage Δ
src/sage/rings/number_field/number_field.py 93.57% <80.00%> (-0.03%) ⬇️
src/sage/rings/number_field/order.py 92.73% <83.33%> (-0.12%) ⬇️

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Mar 14, 2023

Thank you, done.

tscrim
tscrim approved these changes Mar 14, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you.

@yyyyx4 yyyyx4 requested a review from tscrim March 14, 2023 08:16
@yyyyx4
Copy link
Member Author

yyyyx4 commented Mar 14, 2023

It seems I forgot to actually run the test prior to pushing, but the good news is that it already revealed another bug!

tscrim
tscrim approved these changes Mar 14, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Good catch.

@yyyyx4 yyyyx4 force-pushed the public/conductor_methods_for_quadratic_fields branch from cdc7f4a to 48fa9f9 Compare April 2, 2023 02:30
tscrim
tscrim approved these changes Apr 3, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM. If desired, you can do two trivial things:

  1. Pass an iterator to order() in order_of_conductor().
  2. Add a space \neq2 -> \neq 2 as I find it easier to read.

You can ignore these if you want and set the status badge to positive review.

@yyyyx4 yyyyx4 force-pushed the public/conductor_methods_for_quadratic_fields branch from 48fa9f9 to c6461b0 Compare April 5, 2023 02:27
@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 5, 2023 •
edited
Loading

  1. Pass an iterator to order() in order_of_conductor().

This doesn't work without additional code changes: The .order() method explicitly checks for lists and tuples.

@tscrim
Copy link
Collaborator

tscrim commented Apr 5, 2023

I see; I didn't notice that. Thanks for checking.

@yyyyx4 yyyyx4 force-pushed the public/conductor_methods_for_quadratic_fields branch from c6461b0 to 0f61ef0 Compare April 7, 2023 01:19
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 0f61ef0

@vbraun vbraun merged commit 46e46a1 into sagemath:develop Apr 13, 2023
@yyyyx4 yyyyx4 deleted the public/conductor_methods_for_quadratic_fields branch October 8, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscrim tscrim tscrim approved these changes

Assignees
No one assigned
Labels
c: number fields t: enhancement
Projects
None yet
Milestone
sage-10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yyyyx4 @codecov-commenter @tscrim @vbraun @mkoeppe