Opened 7 years ago
Closed 7 years ago
#17090 closed defect (fixed)
Reviewer patch for #16803
Reported by:  jdemeyer  Owned by:  

Priority:  blocker  Milestone:  sage6.4 
Component:  linear algebra  Keywords:  
Cc:  vbraun, mmasdeu, was, SimonKing  Merged in:  
Authors:  Volker Braun, Jeroen Demeyer  Reviewers:  Volker Braun, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  c46a4d5 (Commits, GitHub, GitLab)  Commit:  c46a4d569c3fea2235af809235b5d8eeb6a4b34e 
Dependencies:  Stopgaps: 
Description (last modified by )
Various fixes/improvements/cleanups after #16803.
Change History (42)
comment:1 Changed 7 years ago by
 Cc vbraun mmasdeu was added
comment:2 Changed 7 years ago by
 Cc SimonKing added
comment:3 followup: ↓ 4 Changed 7 years ago by
comment:4 in reply to: ↑ 3 Changed 7 years ago by
comment:5 Changed 7 years ago by
I'd prefer to add it here, there is already too much going on at #16938
comment:6 Changed 7 years ago by
 Branch set to u/vbraun/add_missing_fmpz_init
comment:7 Changed 7 years ago by
 Commit set to df1045910e8324a19b7efb748d884327be4567cc
comment:8 Changed 7 years ago by
 Description modified (diff)
comment:9 followup: ↓ 10 Changed 7 years ago by
The problem is that an assertion won't help against uninitialized variables.
comment:10 in reply to: ↑ 9 Changed 7 years ago by
Replying to jdemeyer:
The problem is that an assertion won't help against uninitialized variables.
And I thought a debug version would have a paranoid memory management, so that the use of uninitialised variables would result in an immediate abort.
comment:11 Changed 7 years ago by
Why not building a static library, by the way?
comment:12 Changed 7 years ago by
Because static libraries suck. If you rebuild flint with debugging, is Sage going to use the new shared library or have the old static library compiled in?
comment:13 followup: ↓ 37 Changed 7 years ago by
And yes, I had hoped that flint had some mechanism to ensure that *_init
is called, but no such luck.
comment:14 Changed 7 years ago by
 Branch changed from u/vbraun/add_missing_fmpz_init to u/jdemeyer/ticket/17090
 Created changed from 10/02/14 13:59:57 to 10/02/14 13:59:57
 Modified changed from 10/02/14 15:28:42 to 10/02/14 15:28:42
comment:15 Changed 7 years ago by
 Commit changed from df1045910e8324a19b7efb748d884327be4567cc to 7b38491f4e99b814b6c238d6c96d7cb263b77e77
comment:16 Changed 7 years ago by
Concerning
+ fmpz_init(z) + fmpz_set_mpz(z, x.value)
Is there no fmpz_init_set
? I know that there is a combined initset function for mpz
, but I don't know about fmpz
.
comment:17 followup: ↓ 18 Changed 7 years ago by
There is:
$ grep fmpz_init_set flint2.4.3/fmpz.h void fmpz_init_set(fmpz_t f, const fmpz_t g) void fmpz_init_set_ui(fmpz_t f, ulong g) void fmpz_init_set_readonly(fmpz_t f, const mpz_t z);
comment:18 in reply to: ↑ 17 ; followup: ↓ 22 Changed 7 years ago by
Replying to vbraun:
There is:
$ grep fmpz_init_set flint2.4.3/fmpz.h void fmpz_init_set(fmpz_t f, const fmpz_t g) void fmpz_init_set_ui(fmpz_t f, ulong g) void fmpz_init_set_readonly(fmpz_t f, const mpz_t z);
I think it should be used.
comment:19 Changed 7 years ago by
 Description modified (diff)
 Summary changed from Add missing fmpz_init() statements after #16803 to Fix issues with #16803
comment:20 Changed 7 years ago by
This branch fixes all valgrind warnings from matrix_integer_dense
(when running benchmark_hnf
)
comment:21 Changed 7 years ago by
 Commit changed from 7b38491f4e99b814b6c238d6c96d7cb263b77e77 to 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619
Branch pushed to git repo; I updated commit sha1. New commits:
8a16a8e  Further cleanup

comment:22 in reply to: ↑ 18 Changed 7 years ago by
 Status changed from new to needs_review
comment:23 Changed 7 years ago by
 Commit changed from 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619 to e5f684b2345e457791a1d878178cb5061764b421
Branch pushed to git repo; I updated commit sha1. New commits:
e5f684b  Remove unused code

comment:24 Changed 7 years ago by
 Commit changed from e5f684b2345e457791a1d878178cb5061764b421 to 9b310031e9d55973b13aa5876756589454a05593
Branch pushed to git repo; I updated commit sha1. New commits:
9b31003  Check 'implementation' argument

comment:25 Changed 7 years ago by
 Commit changed from 9b310031e9d55973b13aa5876756589454a05593 to a2860d835bf647e7f84b876302c06c88eab67c51
Branch pushed to git repo; I updated commit sha1. New commits:
a2860d8  Undo changes to padics from #16803

comment:26 Changed 7 years ago by
comment:27 Changed 7 years ago by
 Commit changed from a2860d835bf647e7f84b876302c06c88eab67c51 to 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85
Branch pushed to git repo; I updated commit sha1. New commits:
342d152  Remove unneeded imports

comment:28 Changed 7 years ago by
 Summary changed from Fix issues with #16803 to Reviewer patch for #16803
comment:29 followups: ↓ 30 ↓ 36 Changed 7 years ago by
Why did you delete the following functions:
_multiply_linbox _multiply_multi_modular _det_pari
comment:30 in reply to: ↑ 29 ; followup: ↓ 31 Changed 7 years ago by
comment:31 in reply to: ↑ 30 Changed 7 years ago by
Replying to jdemeyer:
Replying to was:
Why did you delete the following functions:
_multiply_linbox _multiply_multi_modularThese were simply unused. If you think we should keep them "for reference", there is always the git log.
I'm against removing them. Just because they aren't explicitly used elsewhere in Sage, doesn't mean they aren't useful to have. For example,
 I used them in testing the original patch (having multiple ways of computing products in order to compare) that this is following up on.
 At one time Linbox was the fastest for multiplying matrices (at least in 2007, when Clement Pernet was my postdoc at UW). Now it's terrible, at least as it is built in Sage. Having the function _multiply_linbox makes it easier to test the speed of linbox versus flint (or multimodular) at any point. If you remove it, then suddenly this becomes more difficult. It's entirely plausible that linbox could again become faster than FLINT, and it would be more difficult to know without this function.
 I personally don't trust FLINT, and having an easy alternative to test (or swap out for) matrix multiplication makes me happier. I remember clearly times in the past when FLINT gave flat out wrong answers when multiplying large polynomials (burried deep in some padic cohomology calculations), and  when tracking this down  being able to instrument things so we could easily run the whole big computation with either FLINT or PARI was critical.
comment:32 Changed 7 years ago by
 Commit changed from 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85 to 5b77c7816073fd46c915a05eb6e1461ea02fcd3a
comment:33 Changed 7 years ago by
 Commit changed from 5b77c7816073fd46c915a05eb6e1461ea02fcd3a to 7843e3f1d2021ecf82e3384f0217fce4c5edeec3
Branch pushed to git repo; I updated commit sha1. New commits:
7843e3f  Fix fmpz_mat_t declaration

comment:34 Changed 7 years ago by
 Commit changed from 7843e3f1d2021ecf82e3384f0217fce4c5edeec3 to c46a4d569c3fea2235af809235b5d8eeb6a4b34e
comment:35 Changed 7 years ago by
I am going to leave this branch alone for now, please review! There is another followup at #17094.
comment:36 in reply to: ↑ 29 Changed 7 years ago by
comment:37 in reply to: ↑ 13 ; followup: ↓ 38 Changed 7 years ago by
Replying to vbraun:
And yes, I had hoped that flint had some mechanism to ensure that
*_init
is called, but no such luck.
In C, this simply cannot be done. A variable on the stack starts out in a totally unpredictable state.
comment:38 in reply to: ↑ 37 ; followup: ↓ 39 Changed 7 years ago by
Replying to jdemeyer:
In C, this simply cannot be done. A variable on the stack starts out in a totally unpredictable state.
Thats of course, strictly speaking, true. But I would be happy to take my chances with a padding int being initialized to 0xdeadbeef
and having a random, but usually different, value if it was not initialized.
comment:39 in reply to: ↑ 38 Changed 7 years ago by
Replying to vbraun:
But I would be happy to take my chances with a padding int being initialized to
0xdeadbeef
...if you take care to clear out that 0xdeadbeef
after use and assume that the compiler doesn't optimize that clearing away, I guess it would work.
comment:40 Changed 7 years ago by
 Status changed from needs_review to positive_review
Looks good to me!
comment:41 Changed 7 years ago by
 Reviewers set to Volker Braun, Jeroen Demeyer
comment:42 Changed 7 years ago by
 Branch changed from u/jdemeyer/ticket/17090 to c46a4d569c3fea2235af809235b5d8eeb6a4b34e
 Resolution set to fixed
 Status changed from positive_review to closed
Flint has a
enableassert
configure switch to turn on assertions which should be handy for debugging.