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

sqrt() for QQbar and AA should have a parameter "all" #8802

Closed
JohnCremona opened this issue Apr 28, 2010 · 11 comments
Closed

sqrt() for QQbar and AA should have a parameter "all" #8802

JohnCremona opened this issue Apr 28, 2010 · 11 comments

Comments

@JohnCremona
Copy link
Member

This is inconsistent with other versions of sqrt():

sage: QQbar(2).sqrt()
1.414213562373095?
sage: QQbar(2).sqrt(all=True)

In addition, there should be a parameter "extend" to handle this:

sage: AA(2).sqrt()
1.414213562373095?
sage: AA(-2).sqrt()
1.414213562373095?*I

In the second example, we should not return a root in QQbar unless extend=True.

CC: @robertwb @loefflerd @JohnCremona

Component: algebra

Author: John Cremona, Barinder Banwait

Reviewer: Robert Bradshaw, David Loeffler

Merged: sage-4.4.4.alpha1

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

@sagetrac-barinder
Copy link
Mannequin

sagetrac-barinder mannequin commented Jun 7, 2010

Attachment: trac_8802-sqrt-qqbar.patch.gz

Add all and extend parameters to sqrt for QQbar and AA

@sagetrac-barinder

This comment has been minimized.

@sagetrac-barinder
Copy link
Mannequin

sagetrac-barinder mannequin commented Jun 7, 2010

comment:2

Patch based on 4.4.2 and works fine on 4.4.3. The two problems identified by cremona have been resolved:

sage: QQbar(2).sqrt()
1.414213562373095?
sage: QQbar(2).sqrt(all=True)
[1.414213562373095?, -1.414213562373095?]

The following command

sage: AA(-2).sqrt(extend=False)

returns an error, like it should.

@robertwb
Copy link
Contributor

robertwb commented Jun 7, 2010

comment:4

Mostly looks good. Some minor comments:

  • There's a lot of redundancy between the AA and QQbar cases, perhaps it could be consolidated (this isn't that big of a deal though)
  • You normalize the root order in the all=True case, which I think is good, but the same normalization would probably be worth doing for the all=False case too (I'd be happy if it might negate twice to simplify the logic, as that will be relatively cheap).

@sagetrac-barinder
Copy link
Mannequin

sagetrac-barinder mannequin commented Jun 7, 2010

Attachment: trac_8802-sqrt-qqbar-v2.patch.gz

As before, but addresses the ordering issue raised by robertwb

@JohnCremona
Copy link
Member Author

Attachment: trac_8802-sqrt-qqbar-v3.patch.gz

Fixed docstring format issues and simplified: applies to 4.4.4.alpha0

@JohnCremona
Copy link
Member Author

comment:7

Version 3 of the patch (jointly written by Barinder and John) simplifies the code more and fixes docstring issues. We removed the sign normalisation code, since the pow function already delivers a normalised result.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jun 10, 2010

Author: John Cremona, Barinder Banwait

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jun 10, 2010

Reviewer: Robert Bradshaw, David Loeffler

@robertwb
Copy link
Contributor

comment:9

Looks good to me too.

@mwhansen
Copy link
Contributor

Merged: sage-4.4.4.alpha1

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

4 participants