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

Added Doctests for QuadraticForms methods #6040

Closed
jonhanke opened this issue May 15, 2009 · 12 comments
Closed

Added Doctests for QuadraticForms methods #6040

jonhanke opened this issue May 15, 2009 · 12 comments

Comments

@jonhanke
Copy link

Adding Doctests to bring coverage up to 100%.

CC: @sagetrac-mabshoff @williamstein @tornaria

Component: quadratic forms

Keywords: quadraticform

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

@jonhanke
Copy link
Author

comment:1

Attachment: patch-3__QF_misc_doctests__3.4.1.patch.gz

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.

@jonhanke
Copy link
Author

comment:3

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.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented May 15, 2009

comment:4

Attachment: patch-4__QF_more_doctests__3.4.1.patch.gz

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

Cheers,

Michael

@jonhanke
Copy link
Author

comment:5

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

@jonhanke

This comment has been minimized.

@tornaria
Copy link
Contributor

comment:6

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.

@tornaria
Copy link
Contributor

Attachment: patch-5__QF_reviewer__4.0.alpha0.patch.gz

fix doctests for 4.0.alpha0

@tornaria
Copy link
Contributor

comment:7

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.

@tornaria
Copy link
Contributor

comment:8

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.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented May 18, 2009

comment:9

Add positive review due to Gonzalo.

Cheers,

Michael

@tornaria
Copy link
Contributor

comment:10

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.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented May 19, 2009

comment:11

Merged in Sage 4.0.rc0.

Cheers,

Michael

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

2 participants