Opened 5 years ago
Closed 5 years ago
#19341 closed enhancement (fixed)
Cleaning/Fix in combinat/matrices/hadamard_matrix
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.9 |
Component: | combinatorial designs | Keywords: | |
Cc: | dimpase | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | 1159010 (Commits, GitHub, GitLab) | Commit: | 11590100b6520e833cf8de16732d85407d836214 |
Dependencies: | #19340 | Stopgaps: |
Description
As the title says. Generalizes the existing constructions, and fixes a bug in normalised_hadamard.
Nathann
Change History (19)
comment:1 Changed 5 years ago by
- Branch set to u/ncohen/19341
- Commit set to 858e25929ac6fef40bc762e660acba21f55f0d5d
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
- Commit changed from 858e25929ac6fef40bc762e660acba21f55f0d5d to 83bbce2b3d5e8940971f4fe3196e088d58fc018a
comment:3 Changed 5 years ago by
I pushed a branch at u/vdelecroix/19340 with one commit which creates a function hadamard_matrix_flint using flint.
comment:4 Changed 5 years ago by
Then you will also have ton integrate it into hadamard_matrix
if you want it to be merged with this branch. You can then write a doctest to compare it with the 'current' (i.e. after this branch) implementation of the paley matrices, which should be correct (but slower) now.
Nathann
comment:5 Changed 5 years ago by
please don't use the coersion, unless really needed:
sage: # your implementation: sage: timeit('matrix(ZZ, [[2*((x-y).is_square()-.5) for x in K_list] for y in K_list])') 5 loops, best of 3: 269 ms per loop sage: # a direct one: sage: timeit('matrix(ZZ, [[1 if (x-y).is_square() else -1 for x in K_list] for y in K_list])') 25 loops, best of 3: 27.9 ms per loop
(this timing is for p=3^5
)
comment:6 follow-up: ↓ 8 Changed 5 years ago by
How am I supposed to write it instead?
comment:7 Changed 5 years ago by
O_o
Really?... That's only because of float->ZZ ? O_o
comment:8 in reply to: ↑ 6 Changed 5 years ago by
Replying to ncohen:
How am I supposed to write it instead?
1 if (x-y).is_square() else -1
instead of
2*((x-y).is_square()-.5)
much more readable too... ;-)
comment:9 follow-up: ↓ 11 Changed 5 years ago by
What the hell. Just because I multiply a bool with an integer? What kind of language is that O_o
comment:10 Changed 5 years ago by
Anyway. You are right. But I really did not expect such a diffrence O_o
Nathann
comment:11 in reply to: ↑ 9 Changed 5 years ago by
Replying to ncohen:
What the hell. Just because I multiply a bool with an integer? What kind of language is that
O_o
python_digesting_a_rabbit
well, there is also a float involved...
comment:12 Changed 5 years ago by
- Commit changed from 83bbce2b3d5e8940971f4fe3196e088d58fc018a to 11590100b6520e833cf8de16732d85407d836214
comment:13 follow-up: ↓ 15 Changed 5 years ago by
- Reviewers set to Dima Pasechnik
why are there tests marked # random
?
comment:14 Changed 5 years ago by
by the way, bool
is only a smaller part of the slowness:
sage: timeit('matrix(ZZ, [[1 if (x-y).is_square() else -1 for x in K_list] for y in K_list])') 25 loops, best of 3: 27.9 ms per loop sage: timeit('matrix(ZZ, [[2*((x-y).is_square()-5) for x in K_list] for y in K_list])') 5 loops, best of 3: 57.1 ms per loop sage: timeit('matrix(ZZ, [[2*((x-y).is_square()-.5) for x in K_list] for y in K_list])') 5 loops, best of 3: 265 ms per loop
appearance of float
causes most of the slowness.
comment:15 in reply to: ↑ 13 Changed 5 years ago by
Replying to dimpase:
why are there tests marked
# random
?
the only possible source of randomness here I can imagine is in the construction of
the finite fields. But if you add conway=True
then it's deterministic for sure, for Conway polynomial is unique for any given order...
comment:16 Changed 5 years ago by
The random is indeed caused by the finite field, and the order in which the elements are enumerated. Also these functions are now tested heavily, so whatever is returned by those functions should be what the user expects.
comment:17 Changed 5 years ago by
- Status changed from needs_review to positive_review
OK, let's deal with FLINT matrices on another ticket.
comment:18 Changed 5 years ago by
Yep. At least it is correct now.
Thanks,
Nathann
comment:19 Changed 5 years ago by
- Branch changed from u/ncohen/19341 to 11590100b6520e833cf8de16732d85407d836214
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #19340: Better interface for hadamard_matrix
trac #19340: Pre-existing bug that hid several paley constructions
trac #19341: Cleaning/Fix in combinat/matrices/hadamard_matrix