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

Fix infinite loop bug in doctest from #36581 #38209

Merged
merged 5 commits into from
Jun 22, 2024

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Jun 13, 2024

Discovered by tornaria here.

Sorry for the incorrect implementation! Please merge this quickly, before the next beta (I think).

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

Should "p: CI Fix" be added?

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Please be sure to test this locally:

**********************************************************************
File "src/sage/stats/distributions/discrete_gaussian_lattice.py", line 484, in sage.stats.distributions.discrete_gaussian_lattice.DiscreteGaussianDistributionLatticeSampler.__init__
Failed example:
    v = vector(ZZ, n, (0, 0))
Exception raised:
    Traceback (most recent call last):
      File "/home/travis/sage-build/src/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/travis/sage-build/src/sage/doctest/forker.py", line 1147, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.stats.distributions.discrete_gaussian_lattice.DiscreteGaussianDistributionLatticeSampler.__init__[23]>", line 1, in <module>
        v = vector(ZZ, n, (Integer(0), Integer(0)))
      File "sage/modules/free_module_element.pyx", line 535, in sage.modules.free_module_element.vector
        raise ValueError("incompatible degrees in vector constructor")
    ValueError: incompatible degrees in vector constructor

It subsequently hangs on line 486.

@tscrim
Copy link
Collaborator

tscrim commented Jun 13, 2024

See also the linter.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

My bad. I changed my editor config at some time and must have removed the trailing whitespace highlighting..

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

(I'm fixing the hang(?), give me a second)

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

D.f is not the most descriptive name for a class, so I should refactor it at some point. I will blame it on the original author though 🙈

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Some progress, but now line 501 hangs.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

It doesn't hang for me..? can you give me a seed

@tscrim
Copy link
Collaborator

tscrim commented Jun 13, 2024

78175743252608910157834302274104155990

@tscrim
Copy link
Collaborator

tscrim commented Jun 13, 2024

For the record, I am also testing with --long --verbose

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

I will have a look. I only test with sage -t --verbose without long, but I tested it for 10 times without it looping.. maybe I was lucky

@tscrim
Copy link
Collaborator

tscrim commented Jun 13, 2024

This one seed seems particularly bad; when I tried a few more times, everything passes.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

Okay. My _normalisation_factor_zz prints a warning message that it's a bad approximate right? It's really bad. For the vector v in the test case, it outputs 63.x when the true value is 78.x. That determines the computed expected value, so it's way off -> test case loops infinitely. I will just bump up the BOUND and put a TODO...

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jun 13, 2024

Okay so as it turns out I can't do math. I accidentally coded the solution to $B^N \leq 10^4$ as $B = \log_N(10^4)$. I fixed that and now it works.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you. Then if the bots come back green, then we can set this to a positive review.

Copy link

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

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
    
Discovered by tornaria [here](sagemath#3658
1/files/5c51f54b1a193c3f9964da3e1388efd87623d45c#r1575566025).

Sorry for the incorrect implementation! Please merge this quickly,
before the next beta (I think).
    
URL: sagemath#38209
Reported by: grhkm21
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
    
Discovered by tornaria [here](sagemath#3658
1/files/5c51f54b1a193c3f9964da3e1388efd87623d45c#r1575566025).

Sorry for the incorrect implementation! Please merge this quickly,
before the next beta (I think).
    
URL: sagemath#38209
Reported by: grhkm21
Reviewer(s): Travis Scrimshaw
@vbraun vbraun merged commit c8089eb into sagemath:develop Jun 22, 2024
24 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants