-
Notifications
You must be signed in to change notification settings - Fork 63
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
Modify testset "Generic.Ideal.addition" #1458
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1458 +/- ##
==========================================
+ Coverage 84.60% 84.61% +0.01%
==========================================
Files 110 110
Lines 29379 29379
==========================================
+ Hits 24857 24860 +3
+ Misses 4522 4519 -3 see 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Looking at the CI status it seems that Also I don't think reducing the number of tests will fix this. I would guess there is probably a single example that triggers this. So depending of the index of the example we might just skip this now but it can eventually reappear for a different seed. I would be in favor of fixed ideals. |
With the reduced loops in this PR please try seed 16234.
in the first loop. Edit: That seems to succeed after about 7 minutes and with 22GB of RAM. |
OK, then I agree a fixed list of ideals would be preferable. |
cfad494
to
5f203cd
Compare
Modified to use a fixed list of polynomial ideals in the multivariate cases. |
OK clearly this is not enough: I missed the function |
5f203cd
to
8a2bd35
Compare
I was easily able to reproduce this locally with a loop
where it took 50 minutes in some cases, e.g. seed 14. With the latest tweak, each iteration of the above loops runs in about 15 seconds for me. |
Don't use random examples in the multivariate case, as some of these can lead to *really* horribly bad runtimes. Instead use a fixed list of examples. For the univariate cases and the ideals in Z, I did not change anything so far, as those should not suffer from the same problems (famous last words...)
8a2bd35
to
455827c
Compare
Are you happy with the current state @fingolfin? |
So
and then it proceeds to run other test... until it eventually dies. My guess: it never reduces the To test my theory I inserted a CC @gbaraldi |
@thofma I am happy with the first commit. I am not sure we should keep the second "HACK" commit which inserts a But I am also fine if this gets squashed; or if we just merge the first commit... shrug |
Let's keep it for now |
Depending on the random seed, this test could take forever resp. run out of memory. Restrict it to fewer tests for now.This may not be enough, though; we may need to set a fixed RNG seed in this testset, or just replace the "random" ideals by fixed ones which are known to not exhibit the bad performance.Don't use random examples in the multivariate case, as some of these can
lead to really horribly bad runtimes. Instead use a fixed list of
examples.
For the univariate cases and the ideals in Z, I did not change anything
so far, as those should not suffer from the same problems (famous last
words...)
CC @benlorenz