Opened 4 years ago

Closed 4 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) 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 4 years ago by ncohen

  • Branch set to u/ncohen/19341
  • Commit set to 858e25929ac6fef40bc762e660acba21f55f0d5d
  • Status changed from new to needs_review

New commits:

f465c96trac #19340: Better interface for hadamard_matrix
247a2bbtrac #19340: Pre-existing bug that hid several paley constructions
858e259trac #19341: Cleaning/Fix in combinat/matrices/hadamard_matrix

comment:2 Changed 4 years ago by git

  • Commit changed from 858e25929ac6fef40bc762e660acba21f55f0d5d to 83bbce2b3d5e8940971f4fe3196e088d58fc018a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a5a4bbftrac #19340: Pre-existing bug that hid several paley constructions
83bbce2trac #19341: Cleaning/Fix in combinat/matrices/hadamard_matrix

comment:3 Changed 4 years ago by vdelecroix

I pushed a branch at u/vdelecroix/19340 with one commit which creates a function hadamard_matrix_flint using flint.

comment:4 Changed 4 years ago by ncohen

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 4 years ago by dimpase

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: Changed 4 years ago by ncohen

How am I supposed to write it instead?

comment:7 Changed 4 years ago by ncohen

O_o

Really?... That's only because of float->ZZ ? O_o

comment:8 in reply to: ↑ 6 Changed 4 years ago by dimpase

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: Changed 4 years ago by ncohen

What the hell. Just because I multiply a bool with an integer? What kind of language is that O_o

comment:10 Changed 4 years ago by ncohen

Anyway. You are right. But I really did not expect such a diffrence O_o

Nathann

comment:11 in reply to: ↑ 9 Changed 4 years ago by dimpase

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 4 years ago by git

  • Commit changed from 83bbce2b3d5e8940971f4fe3196e088d58fc018a to 11590100b6520e833cf8de16732d85407d836214

Branch pushed to git repo; I updated commit sha1. New commits:

105e16dtrac #19341: Merged with 6.9
1159010trac #19341: Don't consider booleans as integers. Just don't.

comment:13 follow-up: Changed 4 years ago by dimpase

  • Reviewers set to Dima Pasechnik

why are there tests marked # random ?

comment:14 Changed 4 years ago by dimpase

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 4 years ago by dimpase

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 4 years ago by ncohen

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 4 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, let's deal with FLINT matrices on another ticket.

comment:18 Changed 4 years ago by ncohen

Yep. At least it is correct now.

Thanks,

Nathann

comment:19 Changed 4 years ago by vbraun

  • Branch changed from u/ncohen/19341 to 11590100b6520e833cf8de16732d85407d836214
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.