Ticket #11455 (closed enhancement: fixed)

Opened 2 years ago

Last modified 9 months ago

Add algorithm Magma to Conic methods

Reported by: mstreng Owned by: AlexGhitza
Priority: major Milestone: sage-5.3
Component: algebraic geometry Keywords: magma conic
Cc: florian Work issues:
Report Upstream: N/A Reviewers: Florian Bouyer
Authors: Marco Streng Merged in: sage-5.3.beta2
Dependencies: #10621, #11454, #11456 Stopgaps:

Description (last modified by mstreng) (diff)

This patch adds the option algorithm='magma' to (has_)rational_point for conics.

Apply

Attachments

trac_11455_magma_algorithm_conics.patch Download (10.5 KB) - added by mstreng 22 months ago.
use magma to solve conics
11455.patch Download (11.4 KB) - added by mstreng 16 months ago.
11455.2.patch Download (11.4 KB) - added by mstreng 10 months ago.
11455.3.patch Download (11.4 KB) - added by mstreng 10 months ago.
same as previous one, but with corrected "optional - magma" tag
11455-doctests.patch Download (9.2 KB) - added by mstreng 10 months ago.
apply on top of previous, fixes instable doctests

Change History

comment:1 follow-up: ↓ 2 Changed 2 years ago by mstreng

  • Work issues set to review tickets on which this depends, and (in a separate ticket) implement magma interface for number field elements

Apparently the Magma interface for number field elements doesn't work the way I thought it would: 

sage: L.<s> = QuadraticField(2)
sage: m = magma(s)
sage: m.sage()
NameError: name 's' is not defined

sage: K.<i> = QuadraticField(-1)
sage: m = magma(i).sage(); m
I
sage: m.parent()
Symbolic Ring
sage: K(m)
TypeError: <type 'sage.symbolic.expression.Expression'>

Fraction fields are a problem too: 

sage: R.<x> = QQ[]
sage: y=x^2/x; y.parent()
Fraction Field of Univariate Polynomial Ring in x over Rational Field
sage: m = magma(y)
sage: m.sage()
NameError: name 'x' is not defined
sage: (m.Numerator().sage()/m.Denominator().sage())
x

comment:2 in reply to: ↑ 1 Changed 2 years ago by mstreng

  • Cc mstreng added
  • Dependencies changed from #10621, #11454 to #10621, #11454, #11456
  • Description modified (diff)
  • Work issues review tickets on which this depends, and (in a separate ticket) implement magma interface for number field elements deleted

comment:3 Changed 2 years ago by mstreng

  • Description modified (diff)

Changed 22 months ago by mstreng

use magma to solve conics

comment:4 Changed 22 months ago by mstreng

  • Priority changed from minor to major
  • Status changed from new to needs_review
  • Description modified (diff)

Please review #10621, a student will need #11455 for his project in September.

comment:5 Changed 16 months ago by mstreng

  • Cc mstreng removed
  • Description modified (diff)

comment:6 follow-up: ↓ 7 Changed 16 months ago by mstreng

  • Status changed from needs_review to needs_work
  • Description modified (diff)

Something is wrong, in 5.0.prealpha1, I get 

sage: K.<i> = QuadraticField(-1)
sage: C = Conic(K,[1,1,1])
sage: C.rational_point(algorithm='magma')
NotImplementedError: No correct conversion implemented for converting the Magma point (-i : 1 : 0) on Conic over Number Field with defining polynomial t^2 + 1 over the Rational Field defined by
X^2 + Y^2 + Z^2 to a correct Sage point on self (=Projective Conic Curve over Number Field in i with defining polynomial x^2 + 1 defined by x^2 + y^2 + z^2)

comment:7 in reply to: ↑ 6 Changed 16 months ago by mstreng

Replying to mstreng:

Something is wrong

Here's the reason: 

sage: K = QuadraticField(-1, 'i')
sage: L = NumberField(x^2+1, 'i')
sage: magma(K.gen()).sage() in K
False
sage: magma(K.gen()).sage() in L
True
sage: magma(L.gen()).sage() in K
False
sage: magma(L.gen()).sage() in L
True
sage: magma(L.gen()).sage() == magma(K.gen()).sage()
True

A field loses its embeddings when converted to Magma and back. A new patch is coming up.

comment:8 Changed 16 months ago by mstreng

  • Status changed from needs_work to needs_review
  • Description modified (diff)

comment:9 Changed 16 months ago by mstreng

  • Status changed from needs_review to needs_work

Changed 16 months ago by mstreng

comment:10 Changed 16 months ago by mstreng

  • Status changed from needs_work to needs_review

With this patch on 5.0.prealpha1, all tests pass (except those that already fail on an unpatched 5.0.prealpha1).

comment:11 Changed 15 months ago by davidloeffler

Apply 11455.patch

(for patchbot)

Changed 10 months ago by mstreng

comment:12 Changed 10 months ago by mstreng

  • Cc florian added
  • Description modified (diff)

apply 11455.2.patch (which does not introduce any trailing whitespaces, but is otherwise the same)

comment:13 Changed 10 months ago by florian

  • Status changed from needs_review to positive_review
  • Reviewers set to florian

This has passed all test (including long) in Sage-5.2. I checked how the documentation look and that the patch works as it should (on Sage-5.2)

comment:14 Changed 10 months ago by mstreng

  • Reviewers changed from florian to Florian Bouyer

Thanks Florian

comment:15 Changed 10 months ago by jdemeyer

  • Status changed from positive_review to needs_work

You should use 

# optional - magma

instead of 

# optional, uses magma

Changed 10 months ago by mstreng

same as previous one, but with corrected "optional - magma" tag

comment:16 Changed 10 months ago by mstreng

  • Status changed from needs_work to needs_review
  • Description modified (diff)

apply 11455.3.patch

comment:17 Changed 10 months ago by mstreng

  • Status changed from needs_review to needs_work

oops, with the correct "optional" tag, it turns out a lot of the optional tests fail

Changed 10 months ago by mstreng

apply on top of previous, fixes instable doctests

comment:18 Changed 10 months ago by mstreng

  • Description modified (diff)

comment:19 Changed 10 months ago by mstreng

  • Status changed from needs_work to needs_review

All tests pass! Florian, could you review again?

comment:20 Changed 10 months ago by mstreng

And thanks Jeroen for catching this!

comment:21 Changed 10 months ago by florian

I'll review it again, sorry for letting the #optional, uses magma slip by.

comment:22 Changed 10 months ago by florian

  • Status changed from needs_review to positive_review

Reviewed it from scratch, everything works.

comment:23 Changed 9 months ago by jdemeyer

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-5.3.beta2
Note: See TracTickets for help on using tickets.