Opened 9 years ago

Closed 6 years ago

Last modified 4 years ago

#9466 closed defect (fixed)

square root with all=True should not return ValueError but empty list

Reported by: mstreng Owned by: AlexGhitza
Priority: major Milestone: sage-5.12
Component: algebra Keywords: sd23 sd51 sqrt all
Cc: mderickx, ruckers Merged in: sage-5.12.beta3
Authors: Marco Streng, Sonseeahray Rucker Reviewers: Alejandro Argaez, Angelos Koutsianas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mstreng)

With Sage 4.4.4 and no relevant patches, I got the following:

sage: FiniteField(3)(2).sqrt(all = True)
[]

sage: 2.sqrt(extend = False, all = True)
ValueError: square root of 2 not an integer

sage: FiniteField(next_prime(2^100))(2).sqrt(extend = False, all = True)
ValueError: self must be a square

sage: _.<a>=FiniteField(9)
sage: a.sqrt(extend = False, all = True)
ValueError: must be a perfect square.

At sage days 23 we agreed that square root with all=True should not raise an error. If no square roots exist, then it should return an empty list.

Right now, it returns an empty list for elements of small prime finite fields, but raises an error for integers, elements of large prime finite fields, and elements of non-prime finite fields.

apply

Attachments (5)

my1.patch (829 bytes) - added by ruckers 7 years ago.
Fixes the code: sage: 2.sqrt(extend = False, all = True)
my2.patch (1.4 KB) - added by ruckers 7 years ago.
Fixes the code: sage: FiniteField?(next_prime(2100))(2).sqrt(extend = False, all = True)
9466.2.patch (3.9 KB) - added by mstreng 6 years ago.
9466.3.patch (3.6 KB) - added by mstreng 6 years ago.
9466.patch (3.6 KB) - added by mstreng 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by mstreng

  • Description modified (diff)

Changed 7 years ago by ruckers

Fixes the code: sage: 2.sqrt(extend = False, all = True)

Changed 7 years ago by ruckers

Fixes the code: sage: FiniteField?(next_prime(2100))(2).sqrt(extend = False, all = True)

comment:2 Changed 7 years ago by ruckers

comment:3 Changed 7 years ago by ruckers

I was not able to fix this code.

sage: _.<a>=FiniteField?(9)

sage: a.sqrt(extend = False, all = True)

ValueError?: must be a perfect square.

comment:4 Changed 7 years ago by mstreng

Thanks! That bug has been around for too long :)

Why were you not able to fix the FiniteField(9) case?

Unfortunately, the patches need some more work, for the following three reasons.

  • With my2.patch:
    sage: Zmod(7)(3).sqrt(extend=True)
    sqrt3
    sage: Zmod(7)(3).sqrt(all=True, extend=True)
    []
    
    The second one should either output [sqrt3, -sqrt3] or raise NotImplementedError, but never an empty list.
  • Patches must have your name and email address in them. This is done by putting your email address and name in your .hgrc file as specified here
  • Add an example of the new functionality to the documentation:
    • This helps the user understand what the function does.
    • Through the doctesting framework, this prevents other people from accidentally breaking your added functionality.
    • This is required for your patch to be able to get a positive review. In fact, the manual says that you should also mention the ticket number #9466 with your example.

If you have any questions, let me know!

Changed 6 years ago by mstreng

Changed 6 years ago by mstreng

Changed 6 years ago by mstreng

comment:5 Changed 6 years ago by mstreng

  • Authors set to Marco Streng
  • Cc ruckers added
  • Description modified (diff)
  • Keywords sd23 sd51 sqrt all added

Does anyone know who user "ruckers" is? (s)he should be added to the list of authors

apply 9466.patch

comment:6 Changed 6 years ago by mstreng

  • Authors changed from Marco Streng to Marco Streng and the person with trac account ruckers
  • Status changed from new to needs_review

please review!

comment:7 Changed 6 years ago by akoutsianas

  • Reviewers set to Alejandro Argaez, Angelos Koutsianas
  • Status changed from needs_review to positive_review

The patch passed all the tests for sage 5.11 version.

comment:8 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 4 years ago by kcrisman

  • Authors changed from Marco Streng and the person with trac account ruckers to Marco Streng, Sonseeahray Rucker
Note: See TracTickets for help on using tickets.