Opened 22 months ago
Last modified 16 months ago
#18639 needs_work defect
New class NumberFieldZeroIdeal
Reported by:  cremona  Owned by:  

Priority:  minor  Milestone:  sage6.10 
Component:  number fields  Keywords:  
Cc:  mmasdeu  Merged in:  
Authors:  John Cremona  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/cremona/18639 (Commits)  Commit:  7be38840da684f9110b4a295712a7eb212f94318 
Dependencies:  #18622  Stopgaps: 
Description (last modified by )
There are several issues with the zero ideal in number fields:
sage: K.<a> = NumberField(x^210) sage: K.<a> = NumberField(x^210) sage: 0/K.ideal(a)  Unhandled SIGSEGV: A segmentation fault occurred in Sage. This probably occurred because a *compiled* component of Sage has a bug in it and is not properly wrapped with sig_on(), sig_off(). Sage will now terminate. 
sage: K.<a> = NumberField(x^215) sage: three = K.ideal(3) sage: zero = K(0) sage: three.divides(zero)  Unhandled SIGSEGV: A segmentation fault occurred in Sage. This probably occurred because a *compiled* component of Sage has a bug in it and is not properly wrapped with sig_on(), sig_off(). Sage will now terminate. 
sage: K.<a> = QuadraticField(5) sage: K.ideal(0) * K.ideal(a+1,3) ... PariError: incorrect type in basistoalg (t_MAT)
(and probably more)
I propose to do some redesign and add a new class NumberFieldZeroIdeal
for just the zero ideal. Then most methods of the current NumberFieldIdeal
should probably be moved to either or both of NumberFieldZeroIdeal
/NumberFieldFractionalIdeal
.
Change History (16)
comment:1 Changed 22 months ago by
 Branch set to u/cremona/18639
 Commit set to 30ca3956b9439aba717ed73397a39dea99a26430
 Status changed from new to needs_review
comment:2 Changed 22 months ago by
Wouldn't the proper fix to just allow (0) / I = (0)
for a fractional ideal I
? Can you check if #18622 does that?
comment:3 Changed 22 months ago by
OK, I will check that. Certainly the 0 ideal passes the is_integral() test, and that should be done anyway, as it is already p ossible to multiply the 0 idal with a nonzero (fractional) one.
comment:4 Changed 22 months ago by
 Dependencies set to #186222
 Description modified (diff)
 Status changed from needs_review to needs_work
 Summary changed from I.divides(0) crashes for I a fractional ideal to (0) / I crashes for I a fractional ideal
It's worse with #18622: it gives a segmentation fault.
In any case, I still think that allowing (0)/I
is the proper fix here, so I'm boldly changing the ticket to do that.
comment:5 followup: ↓ 6 Changed 22 months ago by
 Dependencies changed from #186222 to #18622
I see that #18622 has been merged but is not in the current beta, so the branch name has disappeared from that ticket, so I am hoping that schecking out rremote branch trac/u/jdemeyer/ticket/18611 is the right thing to do.
comment:6 in reply to: ↑ 5 Changed 22 months ago by
Replying to cremona:
I see that #18622 has been merged but is not in the current beta, so the branch name has disappeared from that ticket, so I am hoping that schecking out rremote branch trac/u/jdemeyer/ticket/18611 is the right thing to do.
You can always use the commit hash, that should always work:
git fetch trac # or just "git fetch" depending on your configuration git checkout f61588ecabefc84103c4daa40e648b76e6e58828
comment:7 Changed 22 months ago by
 Description modified (diff)
 Summary changed from (0) / I crashes for I a fractional ideal to Fix zero ideal in number fields
There are actually many issues with the zero ideal...
comment:8 followup: ↓ 10 Changed 22 months ago by
 Commit changed from 30ca3956b9439aba717ed73397a39dea99a26430 to 7be38840da684f9110b4a295712a7eb212f94318
Branch pushed to git repo; I updated commit sha1. New commits:
7be3884  #18639: handle division for 0 ideal

comment:9 Changed 22 months ago by
I hope you don't mind that I'm making this ticket much more general than you intended, but there is really not much point in fixing one specific method when maybe the whole approach needs to be changed.
I am now thinking about the following: introduce a new class NumberFieldZeroIdeal
to represent the zero ideal. Then inherit both NumberFieldZeroIdeal
and NumberFieldFractionalIdeal
from NumberFieldIdeal
(which shouldn't have much functionality).
comment:10 in reply to: ↑ 8 Changed 22 months ago by
comment:11 followup: ↓ 12 Changed 22 months ago by
I have done what was suggested. Basically the div function is moved from NumberFieldFractionalIdeal? to NumberFieldIdeal? with appropriate tests. The test from the original patch are included too, to show that the porioginal issue is resolved. I based this on my original branch but also tested it based on #18622.
I'm sure you will find something I did not do right and/or want to do more (I just saw your comment). Feel free to take this over from me 100% as I have other things to do today.
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 22 months ago by
Replying to cremona:
but also tested it based on #18622.
Are you sure? I ask because #18622 actually changes __div__
to _div_
so I find it hard to believe that you didn't get a conflict.
Feel free to take this over from me 100% as I have other things to do today.
Me too :)
What do you think about the approach I propose in 9?
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 22 months ago by
Replying to jdemeyer:
Replying to cremona:
but also tested it based on #18622.
Are you sure? I ask because #18622 actually changes
__div__
to_div_
so I find it hard to believe that you didn't get a conflict.Feel free to take this over from me 100% as I have other things to do today.
Me too :)
I am sure you do! I am in the middle of implementing Kraus's condictoins and global minimal (or almost global minimal) models for elliptic curves over number fields of class number 1, which noone has ever done before. Nearly finished...
What do you think about the approach I propose in 9?
Good idea. I think WIlliam and I considered this way back (see line 10 of the file in question  it was in fact 28 Jan 2008, almost my first ever Sage development session, and took place in a hotel room in New Jersey where he and I had nothing else to do all day...)
comment:14 in reply to: ↑ 13 Changed 22 months ago by
Replying to cremona:
Good idea. I think WIlliam and I considered this way back (see line 10 of the file in question  it was in fact 28 Jan 2008, almost my first ever Sage development session, and took place in a hotel room in New Jersey where he and I had nothing else to do all day...)
I know. The problem is that currently there is really no way to write special code for just the zero ideal. I also think that NumberFieldZeroIdeal
is more explicit about its intentions, which is a good thing.
comment:15 Changed 22 months ago by
 Description modified (diff)
 Summary changed from Fix zero ideal in number fields to New class NumberFieldZeroIdeal
comment:16 Changed 16 months ago by
 Description modified (diff)
 Milestone changed from sage6.8 to sage6.10
New commits:
#18639: ideal divides 0