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.

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

deprecate constructing number-field fractional ideals via orders' .ideal() method #34806

Closed
yyyyx4 opened this issue Nov 29, 2022 · 12 comments · Fixed by #34979
Closed

deprecate constructing number-field fractional ideals via orders' .ideal() method #34806

yyyyx4 opened this issue Nov 29, 2022 · 12 comments · Fixed by #34979
Labels
c: number fields p: major / 3 t: bug
Milestone
sage-10.0

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Nov 29, 2022 •
edited by mkoeppe
Loading

Currently, Sage creates a fractional ideal of the containing number field when calling .ideal() on a maximal order. This is a mathematically questionable choice, but perhaps more importantly, it will inevitably cause inconsistencies once #34198 is done — the same code would start returning wildly different objects depending on whether the given order is maximal or not.

In this ticket, I propose to deprecate the use of .ideal() on a maximal order to create number-field fractional ideals. This will presumably affect lots of user code, so I think it's good to get the deprecation warning in as soon as possible.

Related, deeper issue: #3680

CC: @slel

Component: number fields

Author: Lorenz Panny

Issue created by migration from https://trac.sagemath.org/ticket/34806
@yyyyx4 yyyyx4 added this to the sage-9.8 milestone Nov 29, 2022
@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 29, 2022

Author: Lorenz Panny

@yyyyx4

This comment has been minimized.

Sign in to view
@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 29, 2022

Branch: public/34806

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2022

Commit: 40363f1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

40363f1add deprecation warning for #34806

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

f9c2d64fix some doctest failures

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Changed commit from 40363f1 to f9c2d64

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Changed commit from f9c2d64 to 636af13

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

636af13fix some more doctest failures

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b97b28fadd deprecation warning for #34806

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Changed commit from 636af13 to b97b28f

@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
yyyyx4 added a commit to yyyyx4/sage that referenced this issue Feb 6, 2023
@yyyyx4
add deprecation warning for sagemath#34806
b97b28f
@yyyyx4 yyyyx4 mentioned this issue Feb 6, 2023
Merged
yyyyx4 added a commit to yyyyx4/sage that referenced this issue Feb 8, 2023
@yyyyx4
add deprecation warning for sagemath#34806
f30b8fc
@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 13, 2023

Removed branch from issue description; replaced by PR #34979

yyyyx4 added a commit to yyyyx4/sage that referenced this issue Feb 14, 2023
@yyyyx4
add deprecation warning for sagemath#34806
b36af1b
yyyyx4 added a commit to yyyyx4/sage that referenced this issue Feb 16, 2023
@yyyyx4
add deprecation warning for sagemath#34806
06b9bf4
yyyyx4 added a commit to yyyyx4/sage that referenced this issue Feb 25, 2023
@yyyyx4
add deprecation warning for sagemath#34806
71361e7
@vbraun vbraun closed this as completed in a6eda33 Mar 13, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 13, 2023
@yyyyx4 yyyyx4 mentioned this issue Sep 18, 2024
Merged
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 27, 2024
sagemathgh-38671: construct order ideals by default for number-field …
6f5604f 
…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 issue Sep 28, 2024
sagemathgh-38671: construct order ideals by default for number-field …
5f1105c 
…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
Assignees
No one assigned
Labels
c: number fields p: major / 3 t: bug
Projects
None yet
Milestone
sage-10.0
Development

Successfully merging a pull request may close this issue.

deprecate constructing number-field fractional ideals via orders' .ideal() method yyyyx4/sage
2 participants
@mkoeppe @yyyyx4