Opened 2 years ago

Last modified 2 years ago

#18639 needs_work defect

New class NumberFieldZeroIdeal

Reported by: cremona Owned by:
Priority: minor Milestone: sage-6.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 jdemeyer)

There are several issues with the zero ideal in number fields:

sage: K.<a> = NumberField(x^2-10)
sage: K.<a> = NumberField(x^2-10)
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^2-15)
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 2 years ago by cremona

  • Branch set to u/cremona/18639
  • Commit set to 30ca3956b9439aba717ed73397a39dea99a26430
  • Status changed from new to needs_review

New commits:

30ca395#18639: ideal divides 0

comment:2 Changed 2 years ago by jdemeyer

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 2 years ago by cremona

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 2 years ago by jdemeyer

  • 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 follow-up: Changed 2 years ago by cremona

  • 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 2 years ago by jdemeyer

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 2 years ago by jdemeyer

  • 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 follow-up: Changed 2 years ago by git

  • 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 2 years ago by jdemeyer

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 2 years ago by jdemeyer

Replying to git:

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

7be3884#18639: handle division for 0 ideal

Sorry, but this will conflict with #18622.

comment:11 follow-up: Changed 2 years ago by cremona

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 ; follow-up: Changed 2 years ago by 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 :-)

What do you think about the approach I propose in 9?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by cremona

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 no-one 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 2 years ago by jdemeyer

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 2 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Fix zero ideal in number fields to New class NumberFieldZeroIdeal

comment:16 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-6.8 to sage-6.10
Note: See TracTickets for help on using tickets.