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

Sort #34892 doctest output vectors to fix randomness #38524

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Aug 18, 2024

Reported at #38492 (comment) by @jhpalmieri and @GMS103. Added an extra test because why not.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 18, 2024

By the way, is this intended?

sage: L = IntegralLattice(Matrix(QQ, [[1000, 0], [0, 1]]))
....: L.0.norm()
1

Copy link

github-actions bot commented Aug 18, 2024

Documentation preview for this PR (built with commit 4c18be7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 18, 2024

By the way, is this intended?

sage: L = IntegralLattice(Matrix(QQ, [[1000, 0], [0, 1]]))
....: L.0.norm()
1

This is because the given matrix is interpreted as the Gram matrix of the lattice: The basis vectors are always (1,0) and (0,1), and their pairwise inner products are specified by the matrix entries. To compute the norm, you'd have to do L.0 * L.gram_matrix() * L.0, I suppose. Perhaps there's a shortcut.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 18, 2024

Indeed, presumably this is not optimal. The parent of this vector is set to the correct lattice, but the .norm() method behaves the same as for the standard Euclidean lattice regardless of the actual parent. This seems like something worth fixing.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 18, 2024

Yeah that's what I was thinking, v.parent() gives the lattice

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your patch (thanks!) will fix the doctest failures, but it also leaves us with tests which basically only check that the method doesn't crash. I'm not sure how to best resolve this. Practically, perhaps we can get away with a sorted() and deal with it properly if/when it breaks again on some other setup?

@jhpalmieri
Copy link
Member

If you don't want to use sorted, you could check membership of some entries. Do you know whether (3, 2, -2, -4) will alway be among the first 20 entries, for example? I don't know the code at all. Are there any meaningful tests you can run on the output?

By the way, is this intended?

sage: L = IntegralLattice(Matrix(QQ, [[1000, 0], [0, 1]]))
....: L.0.norm()
1

This is because the given matrix is interpreted as the Gram matrix of the lattice: The basis vectors are always (1,0) and (0,1), and their pairwise inner products are specified by the matrix entries. To compute the norm, you'd have to do L.0 * L.gram_matrix() * L.0, I suppose. Perhaps there's a shortcut.

Why does the second line start with ....: instead of sage:?

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 20, 2024

By the way, is this intended?

sage: L = IntegralLattice(Matrix(QQ, [[1000, 0], [0, 1]]))
....: L.0.norm()
1

This is because the given matrix is interpreted as the Gram matrix of the lattice: The basis vectors are always (1,0) and (0,1), and their pairwise inner products are specified by the matrix entries. To compute the norm, you'd have to do L.0 * L.gram_matrix() * L.0, I suppose. Perhaps there's a shortcut.

Why does the second line start with ....: instead of sage:?

Because I copied this from my REPL and it's not a doctest.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 20, 2024

For now, I will add change it to a sorted call, and if it breaks in any other architecture then too bad we can figure out from there, perhaps testing membership of a short vector as suggested.

@grhkm21 grhkm21 changed the title Mark #34892 doctests as random Sort #34892 doctest output vectors to fix randomness Aug 20, 2024
@grhkm21 grhkm21 requested a review from yyyyx4 August 20, 2024 16:38
@jhpalmieri
Copy link
Member

By the way, is this intended?

sage: L = IntegralLattice(Matrix(QQ, [[1000, 0], [0, 1]]))
....: L.0.norm()
1

This is because the given matrix is interpreted as the Gram matrix of the lattice: The basis vectors are always (1,0) and (0,1), and their pairwise inner products are specified by the matrix entries. To compute the norm, you'd have to do L.0 * L.gram_matrix() * L.0, I suppose. Perhaps there's a shortcut.

Why does the second line start with ....: instead of sage:?

Because I copied this from my REPL and it's not a doctest.

Sorry, I thought you were quoting from the code.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 21, 2024

From CI:

sage -t --random-seed=247508190430796794056562297888034052375 src/sage/modules/free_quadratic_module_integer_symmetric.py
**********************************************************************
File "src/sage/modules/free_quadratic_module_integer_symmetric.py", line 1604, in sage.modules.free_quadratic_module_integer_symmetric.FreeQuadraticModule_integer_symmetric.enumerate_short_vectors
Failed example:
    sorted(vecs, key=lambda v: L(v).inner_product(L(v)))
Expected:
    [(1, -1, 1, 0), (2, -2, 2, 0), (3, -3, 3, 0), (0, 3, 2, 4), (1, 2, 3, 4),
     (4, 4, 1, 0), (3, 2, -2, -4), (3, 5, 0, 0), (4, 1, -1, -4), (-1, 4, 1, 4),
     (2, 1, 4, 4), (5, 3, 2, 0), (2, 3, -3, -4), (2, 6, -1, 0), (5, 0, 0, -4),
     (-2, 5, 0, 4), (4, -4, 4, 0), (6, 2, 3, 0), (1, 4, -4, -4), (3, 0, 5, 4)]
Got:
    [(1, -1, 1, 0),
     (2, -2, 2, 0),
     (3, -3, 3, 0),
     (0, 3, 2, 4),
     (1, 2, 3, 4),
     (4, 4, 1, 0),
     (3, 2, -2, -4),
     (3, 5, 0, 0),
     (4, 1, -1, -4),
     (-1, 4, 1, 4),
     (2, 1, 4, 4),
     (5, 3, 2, 0),
     (2, 3, -3, -4),
     (5, 0, 0, -4),
     (2, 6, -1, 0),
     (-2, 5, 0, 4),
     (4, -4, 4, 0),
     (6, 2, 3, 0),
     (1, 4, -4, -4),
     (3, 0, 5, 4)]
**********************************************************************
1 item had failures:
   1 of  11 in sage.modules.free_quadratic_module_integer_symmetric.FreeQuadraticModule_integer_symmetric.enumerate_short_vectors
    [248 tests, 1 failure, 1.97 s]

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 21, 2024

I'll sort it by the norm and the original vector then

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 27, 2024
…andomness

    
Reported at
sagemath#38492 (comment) by
@jhpalmieri and @GMS103. Added an extra test because why not.
    
URL: sagemath#38524
Reported by: grhkm21
Reviewer(s): Lorenz Panny
@grhkm21 grhkm21 added the p: CI Fix merged before running CI tests label Aug 28, 2024
@vbraun vbraun merged commit 5b55d0d into sagemath:develop Sep 3, 2024
18 of 21 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants