changed seed to make nth-root usable #189
Conversation
|
While rebasing I took the liberty to take the #190 (comment) of @minad into account |
| * 7695055368100873513971520489282333897557005032980617084180385070\ | ||
| * 39530690836861445204045548919181252491201760744537260787 x | ||
| */ | ||
| "4n9cbk886QtLQmofprid3l2Q0GD8Yv979Lh8BdZkFE8g2pDUUSMBET/+M/YFyVZ3mBp", |
There was a problem hiding this comment.
Why are you writing the numbers in base64 and add them in decimal as a comment? Either write them only in base64 or just write them in decimal which I would prefer.
There was a problem hiding this comment.
Why are you writing the numbers in base64 and add them in decimal as a comment?
For the same reason I left the tracing puts("DDD") in it ;-)
I calculated them in Pari/gp and converted them later with an external program to shorten the code lines. It was just a list that I ticked off (the little "x" at the end of the decimal numbers is that tick), not meant as a translation.
[...] or just write them in decimal which I would prefer.
I just don't like the overly long lines (not every compiler accepts string constants in multiple parts). OCD maybe?
There was a problem hiding this comment.
Yes, but you can also split the decimal constants?
There was a problem hiding this comment.
Wat? I didn't read. Not every compiler supports multiple string constants?! Those are unsupported. I hardly believe that tommath would compile with such a thing since it is quite common practice to stringize and concatenate in macros.
There was a problem hiding this comment.
Not every compiler supports multiple string constants?
No, " multiple string constants " is not the right nomenclature, I mean things like this, taking printf as an example
printf( "this shall mimic a %s long "
"line, cut in %c\n", "very", '2');
I vividly remember all the curses I uttered, although it was a long time ago. Nevertheless:
Those [compilers] are unsupported.
The documentation has no restrictions in that regard. It includes the old Borland C compiler and even MSVC is supported. We should set some strict limits, e.g (versions #s arbitrary): GCC > 3, clang > 3.5, MSVC > 2012, icc > 19 and so on (mingw, djgpp, …), document them and test them. The exact version #s are open for discussion, of course. But not here, I think it deservers its own thread, pardon, issue.
There was a problem hiding this comment.
@czurnieden Could you describe a bit more clearly what issue is being solved here? Is there a convergence problem with the old seed?
demo/test.c
Outdated
| @@ -1284,7 +1284,8 @@ static int test_mp_incr(void) | |||
| goto LTM_ERR; | |||
| } | |||
| a.sign = MP_ZPOS; | |||
| if (mp_cmp_d(&a, MP_MASK) != MP_EQ) {puts("DDD"); | |||
| if (mp_cmp_d(&a, MP_MASK) != MP_EQ) { | |||
| puts("DDD"); | |||
There was a problem hiding this comment.
To paraphrase @sjaeckel : I should have gone to bed a long time before that! ;-)
The old seed makes it abysmally slow. It is just not usable with that seed. See this "paper" for some illustrated details. |
|
Uhm, can somebody here tell me what Travis was about to tell me but didn't? |
oops is your confusion caused by me canceling the build job? :) |
Well, as someone who broke the build not only once… ;-) What is it "Conflicting files demo/test.c"? Ah, I see, have already resolved it while applying @minad 's suggestions, just waiting for the local tests to finish to push it. EDIT: The program used to convert the numbers used in teh test for n-root is at https://gist.github.com/czurnieden/b55b09f768fb45db58f3b65b07b2f7b1 now |
czurnieden
force-pushed
the
bugfix-n-root
branch
from
c5da6a2 to
64701cb
Compare
A concrete example is #201 timings of the isolated mp_expt test which checks the result of with orig. n-root: killed after 75 minutes [1] found a bug in that version, fix not yet pushed but used in that test |
|
as you're online @czurnieden I'd say we start with this branch to be rebased and then merged next ...or #204 ...whoever is faster |
czurnieden
force-pushed
the
bugfix-n-root
branch
from
64701cb to
8f6e013
Compare
|
can you please squash those fixup commits together and force-push? the "workaround" commit should stay so it can be reverted in #207 |
czurnieden
force-pushed
the
bugfix-n-root
branch
from
39c658d to
8312296
Compare
@sjaeckel Argh, only the fixes you said. Sorry, squashed too much, I'm afraid. |
perfect like that |
Old implementation made it unusable, especially for tests for (perfect) power which test all prime indices up to log_2(n). This patch does not change the API. It increases the binary size by ca. 450 bytes, about 16%.