Opened 8 years ago
Closed 8 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 8 years ago by
Cc:  vbraun mmasdeu was added 

comment:2 Changed 8 years ago by
Cc:  SimonKing added 

comment:3 followup: 4 Changed 8 years ago by
comment:4 Changed 8 years ago by
comment:5 Changed 8 years ago by
I'd prefer to add it here, there is already too much going on at #16938
comment:6 Changed 8 years ago by
Branch:  → u/vbraun/add_missing_fmpz_init 

comment:7 Changed 8 years ago by
Commit:  → df1045910e8324a19b7efb748d884327be4567cc 

comment:8 Changed 8 years ago by
Description:  modified (diff) 

comment:9 followup: 10 Changed 8 years ago by
The problem is that an assertion won't help against uninitialized variables.
comment:10 Changed 8 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:12 Changed 8 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 8 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 8 years ago by
Branch:  u/vbraun/add_missing_fmpz_init → u/jdemeyer/ticket/17090 

Created:  Oct 2, 2014, 1:59:57 PM → Oct 2, 2014, 1:59:57 PM 
Modified:  Oct 2, 2014, 3:28:42 PM → Oct 2, 2014, 3:28:42 PM 
comment:15 Changed 8 years ago by
Commit:  df1045910e8324a19b7efb748d884327be4567cc → 7b38491f4e99b814b6c238d6c96d7cb263b77e77 

comment:16 Changed 8 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 8 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 followup: 22 Changed 8 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 8 years ago by
Description:  modified (diff) 

Summary:  Add missing fmpz_init() statements after #16803 → Fix issues with #16803 
comment:20 Changed 8 years ago by
This branch fixes all valgrind warnings from matrix_integer_dense
(when running benchmark_hnf
)
comment:21 Changed 8 years ago by
Commit:  7b38491f4e99b814b6c238d6c96d7cb263b77e77 → 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619 

Branch pushed to git repo; I updated commit sha1. New commits:
8a16a8e  Further cleanup

comment:22 Changed 8 years ago by
Status:  new → needs_review 

comment:23 Changed 8 years ago by
Commit:  8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619 → e5f684b2345e457791a1d878178cb5061764b421 

Branch pushed to git repo; I updated commit sha1. New commits:
e5f684b  Remove unused code

comment:24 Changed 8 years ago by
Commit:  e5f684b2345e457791a1d878178cb5061764b421 → 9b310031e9d55973b13aa5876756589454a05593 

Branch pushed to git repo; I updated commit sha1. New commits:
9b31003  Check 'implementation' argument

comment:25 Changed 8 years ago by
Commit:  9b310031e9d55973b13aa5876756589454a05593 → a2860d835bf647e7f84b876302c06c88eab67c51 

Branch pushed to git repo; I updated commit sha1. New commits:
a2860d8  Undo changes to padics from #16803

comment:26 Changed 8 years ago by
Authors:  → Volker Braun, Jeroen Demeyer 

comment:27 Changed 8 years ago by
Commit:  a2860d835bf647e7f84b876302c06c88eab67c51 → 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85 

Branch pushed to git repo; I updated commit sha1. New commits:
342d152  Remove unneeded imports

comment:28 Changed 8 years ago by
Summary:  Fix issues with #16803 → Reviewer patch for #16803 

comment:29 followups: 30 36 Changed 8 years ago by
Why did you delete the following functions:
_multiply_linbox _multiply_multi_modular _det_pari
comment:30 followup: 31 Changed 8 years ago by
comment:31 Changed 8 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 8 years ago by
Commit:  342d1520fbdb36041b1dc7e5a65c62e91dfb1e85 → 5b77c7816073fd46c915a05eb6e1461ea02fcd3a 

comment:33 Changed 8 years ago by
Commit:  5b77c7816073fd46c915a05eb6e1461ea02fcd3a → 7843e3f1d2021ecf82e3384f0217fce4c5edeec3 

Branch pushed to git repo; I updated commit sha1. New commits:
7843e3f  Fix fmpz_mat_t declaration

comment:34 Changed 8 years ago by
Commit:  7843e3f1d2021ecf82e3384f0217fce4c5edeec3 → c46a4d569c3fea2235af809235b5d8eeb6a4b34e 

comment:35 Changed 8 years ago by
I am going to leave this branch alone for now, please review! There is another followup at #17094.
comment:36 Changed 8 years ago by
comment:37 followup: 38 Changed 8 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 followup: 39 Changed 8 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 Changed 8 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:41 Changed 8 years ago by
Reviewers:  → Volker Braun, Jeroen Demeyer 

comment:42 Changed 8 years ago by
Branch:  u/jdemeyer/ticket/17090 → c46a4d569c3fea2235af809235b5d8eeb6a4b34e 

Resolution:  → fixed 
Status:  positive_review → closed 
Flint has a
enableassert
configure switch to turn on assertions which should be handy for debugging.