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.

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

Add algorithm Magma to Conic methods #11455

Closed
mstreng opened this issue Jun 9, 2011 · 36 comments
Closed

Add algorithm Magma to Conic methods #11455

mstreng opened this issue Jun 9, 2011 · 36 comments

Comments

@mstreng
Copy link

mstreng commented Jun 9, 2011

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

Apply

Depends on #10621
Depends on #11454
Depends on #11456

CC: @sagetrac-florian

Component: algebraic geometry

Keywords: magma conic

Author: Marco Streng

Reviewer: Florian Bouyer

Merged: sage-5.3.beta2

Issue created by migration from https://trac.sagemath.org/ticket/11455

@mstreng
Copy link
Author

mstreng commented Jun 9, 2011

comment:1

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

@mstreng
Copy link
Author

mstreng commented Jun 9, 2011

Work Issues: review tickets on which this depends, and (in a separate ticket) implement magma interface for number field elements

@mstreng
Copy link
Author

mstreng commented Jun 9, 2011

Changed work issues from review tickets on which this depends, and (in a separate ticket) implement magma interface for number field elements to none

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Jun 9, 2011

Changed dependencies from #10621, #11454 to #10621, #11454, #11456

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Aug 18, 2011

Attachment: trac_11455_magma_algorithm_conics.patch.gz

use magma to solve conics

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Aug 18, 2011

comment:4

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

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Feb 3, 2012

comment:6

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)

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Feb 3, 2012

comment:7

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.

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Feb 3, 2012

Attachment: 11455.patch.gz

@mstreng
Copy link
Author

mstreng commented Feb 3, 2012

comment:10

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

@mstreng
Copy link
Author

mstreng commented Aug 5, 2012

Changed reviewer from florian to Florian Bouyer

@mstreng
Copy link
Author

mstreng commented Aug 5, 2012

comment:14

Thanks Florian

@jdemeyer
Copy link

jdemeyer commented Aug 6, 2012

comment:15

You should use

# optional - magma

instead of

# optional, uses magma

@mstreng
Copy link
Author

mstreng commented Aug 6, 2012

Attachment: 11455.3.patch.gz

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

@mstreng
Copy link
Author

mstreng commented Aug 6, 2012

comment:16

apply 11455.3.patch

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Aug 6, 2012

comment:17

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

@mstreng
Copy link
Author

mstreng commented Aug 6, 2012

Attachment: 11455-doctests.patch.gz

apply on top of previous, fixes instable doctests

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Aug 6, 2012

comment:19

All tests pass! Florian, could you review again?

@mstreng
Copy link
Author

mstreng commented Aug 6, 2012

comment:20

And thanks Jeroen for catching this!

@sagetrac-florian
Copy link
Mannequin

sagetrac-florian mannequin commented Aug 7, 2012

comment:21

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

@sagetrac-florian
Copy link
Mannequin

sagetrac-florian mannequin commented Aug 7, 2012

comment:22

Reviewed it from scratch, everything works.

@jdemeyer
Copy link

Merged: sage-5.3.beta2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants