Opened 9 years ago

Closed 9 years ago

#6040 closed enhancement (fixed)

[with patch, positive review] Added Doctests for QuadraticForms methods

Reported by: jonhanke Owned by: justin
Priority: major Milestone: sage-4.0
Component: quadratic forms Keywords: quadraticform
Cc: mabshoff, wstein, tornaria Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jonhanke)

Adding Doctests to bring coverage up to 100%.

Attachments (3)

patch-3__QF_misc_doctests__3.4.1.patch (37.9 KB) - added by jonhanke 9 years ago.
patch-4__QF_more_doctests__3.4.1.patch (15.2 KB) - added by jonhanke 9 years ago.
patch-5__QF_reviewer__4.0.alpha0.patch (4.4 KB) - added by tornaria 9 years ago.
fix doctests for 4.0.alpha0

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by jonhanke

comment:1 Changed 9 years ago by jonhanke

Note: There are currently two broken doctests in this patch (using the older routine IsPadic? Square()), which should resolve themselves once Cremona's patch (Ticket #5834) is applied.

comment:2 Changed 9 years ago by mabshoff

  • Summary changed from Added Doctests for QuadraticForms methods to [with patch, needs review] Added Doctests for QuadraticForms methods

comment:3 Changed 9 years ago by jonhanke

Additional patch to bring QuadraticForm? doctests to 100%.

Known Issues:

Several doctests fail because of the need for hilbert_symbol() to accept rational numbers, and similarly for IsPadicSquare?(). These should be addressed by the changes made in Ticket #5834.

Changed 9 years ago by jonhanke

comment:4 Changed 9 years ago by mabshoff

Together with #5954 this brings coverage in the QF code to 100%.

Cheers,

Michael

comment:5 Changed 9 years ago by jonhanke

  • Description modified (diff)

Also the patch in Ticket #6037 (rewrite and careful documentation of local density routines) is related to getting the doctest coverage to 100%.

comment:6 Changed 9 years ago by tornaria

So, it turns out there are 4 patches in this series, and they must be applied in order. In particular, patch-3 depends on patch-2, which is at #6037, but I misunderstood that. The correct sequence is to apply patch-1 in #5954, then patch-2 in #6037, then patch-3 and patch-4 in this ticket.

If that order is followed, the patch sequence applies cleanly to 3.4.1 as well as 4.0.alpha0.

Changed 9 years ago by tornaria

fix doctests for 4.0.alpha0

comment:7 Changed 9 years ago by tornaria

Some doctests were broken on 4.0.alpha0 + patch-1 (#5954) + patch-2 (#6037) + patch-3 + patch-4.

All doctests pass for me when adding on top of that

Note that the patch-3__QF_misc_doctests__4.0.alpha0.patch I uploaded earlier is a mistake, just ignore it.

comment:8 Changed 9 years ago by tornaria

Note: the patch-5 also adds a few "#long time" comments to file quadratic_form__local_representation_conditions.py. Skipping these reduces the time for doctesting the whole file from 48 s to 20 s.

comment:9 Changed 9 years ago by mabshoff

  • Summary changed from [with patch, needs review] Added Doctests for QuadraticForms methods to [with patch, positive review] Added Doctests for QuadraticForms methods

Add positive review due to Gonzalo.

Cheers,

Michael

comment:10 Changed 9 years ago by tornaria

I'm ok with the positive review. I'll list the renamed/removed functions as for the other tickets, and I'll post a ticket to add compatibility functions with deprecation warnings.

  • hanke_mass__maximal was removed.
  • GHY_mass_maximal was renamed GHY_mass__maximal.
  • conway_generic_mass was removed.
  • conway_p_mass_adjustment was removed.
  • local_diagonal was removed.
  • find_primitive_p_divisible_vector__all was removed.

comment:11 Changed 9 years ago by mabshoff

  • Milestone changed from sage-4.0.1 to sage-4.0
  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 4.0.rc0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.