Opened 4 years ago

Closed 4 years ago

#16879 closed enhancement (fixed)

OA caching in C

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.4
Component: combinatorial designs Keywords:
Cc: vdelecroix Merged in:
Authors: Nathann Cohen Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: d7129d6 (Commits) Commit: d7129d6e80c8d3eb11d09e2cd620882a14d6e73d
Dependencies: #16875 Stopgaps:

Description

Caching of OA constructions in C. Less space, and a bit faster.

Nathann

Change History (46)

comment:1 Changed 4 years ago by ncohen

  • Branch set to public/16879
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by git

  • Commit set to f13a4fd01da823289aedecf029277f515eac29f2

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

f7afe6ftrac #16846: Broken doctests
fd1bbc6trac #16846: Merge with updated #16817
c66c19btrac #16846: review
361c403trac #16868: A real difference matrix has k columns
ab25059trac #16870: Use float("inf") instead of Infinity
583622dtrac #16870: remove an import
a64c967trac #16875: move imports
1b36757trac #16875: set the cache when we call difference_matrix
19eeb7ftrac #16879: OA caching in C
f13a4fdtrac #16879: Rename orthogonal_array.py to .pyx

comment:3 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_info

Hello,

The cache part is cool!

I am not confident of having everything switched into Cython. It is much harder to debug and can also makes things slower. Especially with functions like wilson_construction, OA_n_times_2_pow_c_from_matrix... Why not only switch to C for the cache part? The function orthogonal_array might also move and we might also recreate the orthogonal_array_available from #16875.

In the function orthogonal_array, I do not remember what was the point of the variable may_be_availale in orthogonal_arrays... And I am confused by the way it is used: if it is False then there is no need to go further (but it concerns only the case when existence=False).

Vincent

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by ncohen

Yo !

The cache part is cool!

Yep.

I am not confident of having everything switched into Cython.

I don't like it either. And it takes a lifetime to compile.

It is much harder to debug and can also makes things slower.

Really ? Do you have an example ? O_o

Why not only switch to C for the cache part?

Well, I can't cimport a cdef function in a .py file, but perhaps that does not matter. I will move th caching code to designs_pyx.

The function orthogonal_array might also move and we might also recreate the orthogonal_array_available from #16875.

I am thinking about that. What is the difference between this orthogonal_array_available and the current orthogonal_array ? I don't talk about the user interface, just about speed. When the answer is cached (most frequent case), what is it that orthogonal_array_is_available does that orthogonal_array does not ? Only check that k is not None ?

In the function orthogonal_array, I do not remember what was the point of the variable may_be_availale in orthogonal_arrays... And I am confused by the way it is used: if it is False then there is no need to go further (but it concerns only the case when existence=False).

No need to go further to test constructions. But there may be non-existence proofs implemented. That was the logic.

Nathann

comment:5 Changed 4 years ago by ncohen

I just uploaded another branch at public/16879b. It moves the caching functions to designs_pyx instead.

Nathann

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

Replying to ncohen:

Yo !

I am not confident of having everything switched into Cython.

I don't like it either. And it takes a lifetime to compile.

It is much harder to debug and can also makes things slower.

Really ? Do you have an example ? O_o

Almost anything:

def f():
    from sage.rings.arith import is_prime
    from sage.quadratic_forms.extras import is_triangular_number

    for i in range(2,1000):
        is_prime(i)
        is_triangular_number(i)

then I got

sage: timeit("f_python()", number=100)
100 loops, best of 3: 12.7 ms per loop
sage: timeit("f_cython()", number=100)
100 loops, best of 3: 12.9 ms per loop

Not very significant, but still.

Why not only switch to C for the cache part?

Well, I can't cimport a cdef function in a .py file, but perhaps that does not matter. I will move th caching code to designs_pyx.

That's why we can move orthogonal_array as well.

The function orthogonal_array might also move and we might also recreate the orthogonal_array_available from #16875.

I am thinking about that. What is the difference between this orthogonal_array_available and the current orthogonal_array ? I don't talk about the user interface, just about speed. When the answer is cached (most frequent case), what is it that orthogonal_array_is_available does that orthogonal_array does not ? Only check that k is not None ?

If orthogonal_array is cythonized... not much. Otherwise, function calls.

Vincent

comment:7 Changed 4 years ago by vdelecroix

with public/16879b

Error compiling Cython file:
------------------------------------------------------------
_OA_cache = <cache_entry *> sage_malloc(2*sizeof(cache_entry))
            ^
------------------------------------------------------------

sage/combinat/designs/designs_pyx.pyx:509:13: 'cache_entry' is not a type identifier

I guess you forgot a git add or something.

Vincent

comment:8 Changed 4 years ago by ncohen

Right ! I just updated the branch.

Nathann

comment:9 follow-up: Changed 4 years ago by vdelecroix

I think that the -g option is the default, because when you compile with your version you got it twice

gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g ...

comment:10 in reply to: ↑ 9 Changed 4 years ago by vdelecroix

Replying to vdelecroix:

I think that the -g option is the default, because when you compile with your version you got it twice

gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g ...

EDIT: sorry three times!

comment:11 Changed 4 years ago by vdelecroix

And I do not know which optimization is chosen between -O2 and -O3

gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 ...

comment:12 Changed 4 years ago by git

  • Commit changed from f13a4fd01da823289aedecf029277f515eac29f2 to 789533babb6fde4a7745d9b2b846b4957dc9b12e

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

789533btrac #16879: OA caching in C

comment:13 Changed 4 years ago by ncohen

How is this for you ? I plan on modifying orthogonal_array_recursive now, and turn it into a .pyx file. The only time-consuming operations that occur there are the find_* functions which may benefit from C integer variables.

Plus I plan on writing a small function cdef bint _is_available(k,n) there that will 1) check the cache 2) call orthogonal_array(k,n,existence=1) if needed: the point is that because this function is a cdef one, for most calls no Python will be involved at all.

Tell me what you think of this patch, for I cannot begin that if I am not sure that the cache function stay in designs_pyx.

Have fuuuuuun !

Nathann

comment:14 Changed 4 years ago by vdelecroix

After:

  • moving orthogonal_array to designs_pyx
  • setting _OA_cache* to cdef functions
  • creating a orthogonal_array_available

I got 1sec on the first 1200 MOLS.

(I am running the tests right now)

comment:15 Changed 4 years ago by vdelecroix

Conclusion:

  • it is definitely good to switch to C for the cache
  • for that ticket, it would be better to switch the _OA_cache* to cdef and move at the same time orthogonal_array into designs_pyx (I can push my commit without the orthogonal_array_available)
  • to go further it might be a good idea to try the cythonization of the find-part of orthogonal_array_recursive

Vincent

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:16 follow-up: Changed 4 years ago by ncohen

....

Well.. That's kind of what I told you I would do when you will tell me that this patch is okay ?...

Though I wanted to leave orthogonal array in its file. As the is_available c function I want to write in the recursive file makes it useless.

Nathann

comment:17 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by vdelecroix

Replying to ncohen:

....

Well.. That's kind of what I told you I would do when you will tell me that this patch is okay ?...

Though I wanted to leave orthogonal array in its file. As the is_available c function I want to write in the recursive file makes it useless.

Right now, the branch public/16879b looks good except that you should remove

+              extra_compile_args = ['-g'],

The diff is not that big, why not doing everything here (i.e. switching to cython for the find-part of orthogonal_array_recursive)?

Vincent

comment:18 in reply to: ↑ 17 Changed 4 years ago by ncohen

Right now, the branch public/16879b looks good except that you should remove

+              extra_compile_args = ['-g'],

I pused that to this ticket's branch earlier.

The diff is not that big, why not doing everything here (i.e. switching to cython for the find-part of orthogonal_array_recursive)?

I don't mind, I just split stuff to make it easier for you to review. I'll do that now.

Nathann

comment:19 Changed 4 years ago by ncohen

Not as impressive as I had hoped. Without the branch it takes 13s to go to n=1000. With the branch it takes 9.65s.

Of course with your prime power improvement it takes 3.4s to go to 1000. and 1.3s for 600.

Okay, let's say that it is a win :-P

Nathann

comment:20 Changed 4 years ago by git

  • Commit changed from 789533babb6fde4a7745d9b2b846b4957dc9b12e to 5b1977dca2ad045051e337f0f96c860ce245fdb6

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

6fbe31btrac #16879: orthogonal_array_recursive.py -> pyx
5b1977dtrac #16879: a is_available function in orthogonal_arrays_recursive

comment:21 follow-up: Changed 4 years ago by vdelecroix

Hello,

More speed up at public/16879c (pass from 14.2sec to 13.6sec for the first 1200 MOLS). Now that is_available is a cdef, it is the fastest to call so we should always try this one first.

What do you think about moving the actual constructions in orthogonal_arrays.py and globally import them in orthogonal_array_recursive.py? I do not like to have them in Cython (it takes lifetime to compile, you get ugly backtrace, ...).

Vincent

comment:22 in reply to: ↑ 21 ; follow-up: Changed 4 years ago by ncohen

Yooooooooooo !

More speed up at public/16879c (pass from 14.2sec to 13.6sec for the first 1200 MOLS). Now that is_available is a cdef, it is the fastest to call so we should always try this one first.

Cool ! By the way wouldn't is_available(q+1,q) be faster than is_prime_power at some point ? :-PPPPPPPPPPPPP

Anyway, if this is_prime_power stuff keeps on slowing us down we will just cache the first 10 000 values and get rid of the problem.

What do you think about moving the actual constructions in orthogonal_arrays.py and globally import them in orthogonal_array_recursive.py? I do not like to have them in Cython (it takes lifetime to compile, you get ugly backtrace, ...).

I agree that it would be better to keep this code in Python for the moment, and I began to implement it by moving the functions to orthogonal_arrays.py, but I would prefer to keep this file for user-exposed functions if possible. And things like "construction_3_3" does not exactly answer that. Aaaaaaand there would be a orthogonal_arrays_recursive.pyx which would contain no constructions at all, sooo...

What would you think of:

1) Creating a file orthogonal_arrays_find_recursive.pyx that contains all the find functions.

2) Rename orthogonal_arrays_recursive to a .py file and keep the constructions there.

Nathann

comment:23 in reply to: ↑ 22 ; follow-up: Changed 4 years ago by vdelecroix

Replying to ncohen:

Yooooooooooo !

More speed up at public/16879c (pass from 14.2sec to 13.6sec for the first 1200 MOLS). Now that is_available is a cdef, it is the fastest to call so we should always try this one first.

Cool ! By the way wouldn't is_available(q+1,q) be faster than is_prime_power at some point ? :-PPPPPPPPPPPPP

Anyway, if this is_prime_power stuff keeps on slowing us down we will just cache the first 10 000 values and get rid of the problem.

It's not the job of sage.combinat.design... and note that I work hard to make is_prime, is_prime_power, prime_range and prime_power_range faster.

What do you think about moving the actual constructions in orthogonal_arrays.py and globally import them in orthogonal_array_recursive.py? I do not like to have them in Cython (it takes lifetime to compile, you get ugly backtrace, ...).

I agree that it would be better to keep this code in Python for the moment, and I began to implement it by moving the functions to orthogonal_arrays.py, but I would prefer to keep this file for user-exposed functions if possible. And things like "construction_3_3" does not exactly answer that. Aaaaaaand there would be a orthogonal_arrays_recursive.pyx which would contain no constructions at all, sooo...

What would you think of:

1) Creating a file orthogonal_arrays_find_recursive.pyx that contains all the find functions.

2) Rename orthogonal_arrays_recursive to a .py file and keep the constructions there.

Perfect... why not orthogonal_arrays_build_recursive.py it would be even clearer.

Vincent

comment:24 in reply to: ↑ 23 Changed 4 years ago by ncohen

Yoooooooo !

It's not the job of sage.combinat.design... and note that I work hard to make is_prime, is_prime_power, prime_range and prime_power_range faster.

I wonder about that: guys who work on number theory may work differently, or work on very large numbers for which it makes no difference. Or use the heuristics directly. What you do is useful for everybody indeed (and clearly for us too), but if we can save some new seconds with a small cache, why not ?

Perfect... why not orthogonal_arrays_build_recursive.py it would be even clearer.

Because the file already exists like that... But yes, why not ? I will upload a commit for that.

Nathann

comment:25 Changed 4 years ago by ncohen

  • Status changed from needs_info to needs_review

comment:26 Changed 4 years ago by git

  • Commit changed from 5b1977dca2ad045051e337f0f96c860ce245fdb6 to 3db376f043c01b5ff58e46a845bb69e704c1eacf

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

613d30atrac #16879: more speed up
4d927f2trac #16879: rename orthogonal_arrays_recursive to orthogonal_arrays_find_recursive
3352371trac #16879: Move constructions from orthogonal_arrays_find to orthogonal_arrays_build (this, and only this)
a42144dtrac #16879: Fix the import statements
3db376ftrac #16879: Fix the doc

comment:27 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_info

Hello,

Why is_available is in orthogonal_arrays_find_recursive instead of designs_pyx? Even if it is cdef you can cimport it.

One commit at public/16879c... it appears that:

  • divisors is really slow and it is better to do a stupid loop and check divisibility (might not be True for integer > 1000)
  • the kind of loop for m in [1]+range(min(k,19), 122): is very badly cythonized
  • after a %crun, we still have to work on find_wilson_decomposition_with_two_truncated_groups and find_recursive_find_construction_3_3

Vincent

comment:28 in reply to: ↑ 27 ; follow-up: Changed 4 years ago by ncohen

Yo !

Why is_available is in orthogonal_arrays_find_recursive instead of designs_pyx? Even if it is cdef you can cimport it.

I don't know if it will be inlined in this case. Probably we don't care.

One commit at public/16879c... it appears that:

  • divisors is really slow and it is better to do a stupid loop and check divisibility (might not be True for integer > 1000)

Okay.

  • the kind of loop for m in [1]+range(min(k,19), 122): is very badly cythonized

Perhaps, but your fix is ugly.

  • after a %crun, we still have to work on find_wilson_decomposition_with_two_truncated_groups and find_recursive_find_construction_3_3

Yep.

What's the problem with only importing the functions when you need them ?

Nathann

comment:29 in reply to: ↑ 28 ; follow-up: Changed 4 years ago by vdelecroix

Replying to ncohen:

Yo !

Why is_available is in orthogonal_arrays_find_recursive instead of designs_pyx? Even if it is cdef you can cimport it.

I don't know if it will be inlined in this case. Probably we don't care.

Let's keep it where it is right now. But I am not sure at that point that we care a C function call.

One commit at public/16879c... it appears that:

  • divisors is really slow and it is better to do a stupid loop and check divisibility (might not be True for integer > 1000)

Okay.

  • the kind of loop for m in [1]+range(min(k,19), 122): is very badly cythonized

Perhaps, but your fix is ugly.

It depends where you look at! The C code of the first version was definitely ugly! Do you have a better idea to make it work?

What's the problem with only importing the functions when you need them ?

It is slower. An import of an already imported library cost (as you saw with the move imports from #16875). Not a lot, but repeated it does!

Vincent

comment:30 in reply to: ↑ 29 ; follow-up: Changed 4 years ago by ncohen

Yo !

Let's keep it where it is right now. But I am not sure at that point that we care a C function call.

I don't think we do either.

It depends where you look at! The C code of the first version was definitely ugly! Do you have a better idea to make it work?

I look at the code I work with. I agree that it may be nice to improve it, but not at that cost, this is ugly.

What's the problem with only importing the functions when you need them ?

It is slower. An import of an already imported library cost (as you saw with the move imports from #16875). Not a lot, but repeated it does!

Show me the cost of those or leave the code as it is.

Nathann

comment:31 in reply to: ↑ 30 ; follow-up: Changed 4 years ago by vdelecroix

Replying to ncohen:

It depends where you look at! The C code of the first version was definitely ugly! Do you have a better idea to make it work?

I look at the code I work with. I agree that it may be nice to improve it, but not at that cost, this is ugly.

I understand, but there is really something to improve here... no problem to remove my modifications but then what?

What's the problem with only importing the functions when you need them ?

It is slower. An import of an already imported library cost (as you saw with the move imports from #16875). Not a lot, but repeated it does!

Show me the cost of those or leave the code as it is.

A very big cost, indeed!! I split the commits into two. The second one is about imports. The following timings refer to %time MOLS_table(60)

  • with the head at 093ace3 (with the imports as you did): 12.9 s
  • with the head at 680d22b (with the import on top): 12.7 s

Vincent

comment:32 in reply to: ↑ 31 ; follow-up: Changed 4 years ago by ncohen

Helloooooo !!!

I understand, but there is really something to improve here... no problem to remove my modifications but then what?

What about forgetting the lower bound ? This trick was there to save calls to orthogonal_array(existence=True) but now that we have this cdef function it's fine and it does not cost very much to check whether there exists a OA(q+15,q). And it will become the the usual for loop afterward, with the 'range' replaced by C code.

A very big cost, indeed!! I split the commits into two. The second one is about imports. The following timings refer to %time MOLS_table(60)

  • with the head at 093ace3 (with the imports as you did): 12.9 s
  • with the head at 680d22b (with the import on top): 12.7 s

God. It can actually be detected ? O_o

Crazy. Okay, so we keep that. Damn imports.

By the way, the % and / and // operators are not translated into pure Cython code as they should. There is still some Python code about handling the /0 and %0 cases. There is a workaround though:

https://github.com/cython/cython/wiki/enhancements-division

Nathann

comment:33 in reply to: ↑ 32 ; follow-up: Changed 4 years ago by vdelecroix

Replying to ncohen:

Helloooooo !!!

I understand, but there is really something to improve here... no problem to remove my modifications but then what?

What about forgetting the lower bound ? This trick was there to save calls to orthogonal_array(existence=True) but now that we have this cdef function it's fine and it does not cost very much to check whether there exists a OA(q+15,q). And it will become the the usual for loop afterward, with the 'range' replaced by C code.

Nope. I made a mistake the first time I tried and just set the lower bound to k+1 instead of 1. And guess what... you win like 2 secs (and all tests pass)! There are three level of loops and adding one test case to the first one costs. I can redo the timings if you wish.

It would be a good thing to see if corner cases (like m=1 or t=1 in Wilson constructions with one or two truncated blocks) are not handled by other constructions. These corner cases seem to be really expansive.

An other idea would be to have only one find function for product + wilson one truncated + wilson two truncated.

A very big cost, indeed!! I split the commits into two. The second one is about imports. The following timings refer to %time MOLS_table(60)

  • with the head at 093ace3 (with the imports as you did): 12.9 s
  • with the head at 680d22b (with the import on top): 12.7 s

God. It can actually be detected ? O_o

Not really. I would have said that 0.02 seconds is not significant (see the timings below). But still you have strange conventions: sometimes you put the import as the first line of the function and sometimes you put the import just before the return. I would prefer to have everything standardized (and I guess just before return is faster).

By the way, the % and / and // operators are not translated into pure Cython code as they should. There is still some Python code about handling the /0 and %0 cases. There is a workaround though:

https://github.com/cython/cython/wiki/enhancements-division

WTF. With pure C variable we still get some crazy Python translations! Cython is sometimes very nice but sometimes really annoying. I dream of a special flag that allows you to say "do not translate this block to anything, this is pure C and I known what I am doing".

Timings:

commit 3db376f043c01b5ff58e46a845bb69e704c1eacf (yours)
MOLS_table(30)    4.66 s
MOLS_table(60)   13.4  s
MOLS_table(90)   26.3  s

commit 093ace30591e1beed6296c9e5bd16afa37188e4b (my first set of optim)
MOLS_table(30)    4.4  s
MOLS_table(60)   12.7  s
MOLS_table(90)   24.6  s

commit 680d22b2d7dba0c39a0953be1532590ec6bd7113 (the imports)
MOLS_table(30)    4.42 s
MOLS_table(60)   12.7  s
MOLS_table(90)   24.2  s

conclusion: the win of moving the imports part is small. Depending on what you think for this ticket I will split my first commit to see what matters.

Vincent

comment:34 in reply to: ↑ 33 Changed 4 years ago by ncohen

Yo !

It would be a good thing to see if corner cases (like m=1 or t=1 in Wilson constructions with one or two truncated blocks) are not handled by other constructions. These corner cases seem to be really expansive.

Hmmm.. I thought I had a way but it turned out to be wrong. 2sc seems a lot though O_o

An other idea would be to have only one find function for product + wilson one truncated + wilson two truncated.

We would only save the time it takes to run the product+wilson constructions. Either way let's do this on another ticket.

Not really. I would have said that 0.02 seconds is not significant (see the timings below). But still you have strange conventions: sometimes you put the import as the first line of the function and sometimes you put the import just before the return. I would prefer to have everything standardized (and I guess just before return is faster).

I did it this way for the brouwer function because I would have had to add 4 different imports in the same function otherwise.

WTF. With pure C variable we still get some crazy Python translations! Cython is sometimes very nice but sometimes really annoying. I dream of a special flag that allows you to say "do not translate this block to anything, this is pure C and I known what I am doing".

We can actually add this at the top of the file (before the module's doc)

# cython: cdivision=True

Other flags are listed there if you are interested: https://github.com/cython/cython/wiki/enhancements-compilerdirectives

conclusion: the win of moving the imports part is small. Depending on what you think for this ticket I will split my first commit to see what matters.

I don't mind the imports, if it can be detected then no problem. I just don't want reviews to become "coding style fights"..... I do mind the 10 lines in exchange for the bad "for', though :-P

Can we keep this as it is until we find a proper way to avoid it ?

Nathann

P.S. : Sorry for the delay, I am kind of on "holidays" this week. But seeing how this goes I feel that I may escape and go back to work much sooner than expected.

comment:35 follow-up: Changed 4 years ago by vdelecroix

New review commit at public/16879c where

  • all functions have declaration cpdef find_XXX(int k, int n):
  • get rid of the IntegerListsLex
  • // replaced with /
  • one occurrence of divisors replaced by a very naive looping

(but neither ugly loops anymore nor moved import)

It is essentially a clean up but I win a bit on the current timings

MOLS_table(30)    4.44 s
MOLS_table(60)   12.9 s
MOLS_table(90)   25   s

If you like the review commit at public/16879c you can set to positive review.

Vincent

comment:36 in reply to: ↑ 35 ; follow-up: Changed 4 years ago by ncohen

  • Status changed from needs_info to needs_review

Yo !

New review commit at public/16879c where

  • all functions have declaration cpdef find_XXX(int k, int n):

Okay. But they are still called "python-style" by find_recursive_construction it seems, for they are stored in a list and everything.

  • get rid of the IntegerListsLex

Cool ! I added a comment to "remember" what this part of the code does.

  • // replaced with /

Okay. It produces the very same C code though.

  • one occurrence of divisors replaced by a very naive looping

(but neither ugly loops anymore nor moved import)

Okayyyyy !

If you like the review commit at public/16879c you can set to positive review.

I added a small commit on top. I have to write an is_quasi_difference_matrix function but I don't enjoy it very much T_T

Nathann

comment:37 Changed 4 years ago by git

  • Commit changed from 3db376f043c01b5ff58e46a845bb69e704c1eacf to cc0e804efc31f83e5be7fcc694416ae4a78114e7

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

da9b5abtrac #16879: speed up
cc0e804trac #16879: Trivial stuff

comment:38 in reply to: ↑ 36 ; follow-up: Changed 4 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

Replying to ncohen:

Yo !

New review commit at public/16879c where

  • all functions have declaration cpdef find_XXX(int k, int n):

Okay. But they are still called "python-style" by find_recursive_construction it seems, for they are stored in a list and everything.

Right. Unwraping the loop makes it faster... but I am sure you will not like that ;-)

  • get rid of the IntegerListsLex

Cool ! I added a comment to "remember" what this part of the code does.

  • // replaced with /

Okay. It produces the very same C code though.

But I put the flag at the top! The problem is that we do not see the cythonization command.

  • one occurrence of divisors replaced by a very naive looping

(but neither ugly loops anymore nor moved import)

Okayyyyy !

If you like the review commit at public/16879c you can set to positive review.

Let us go and play with something else. We will come back to that kind of optimization later when we will care about MOLS_table(200)...

Vincent

comment:39 in reply to: ↑ 38 Changed 4 years ago by ncohen

Yo !

Right. Unwraping the loop makes it faster... but I am sure you will not like that ;-)

I rather doubt that it would produce any significant improvement.

But I put the flag at the top! The problem is that we do not see the cythonization command.

Nononono, the flag works ! And you can see the C code that it produces with 'sage -cython -a file.pyx' (produces a.html file). What I mean is that with this flag, having '' or '/' is the very same and that changing them all in the code is useless as far as the C code is concerned.

Let us go and play with something else. We will come back to that kind of optimization later when we will care about MOLS_table(200)...

+1

Nathann

comment:40 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge failure in src/sage/combinat/designs/database.py

comment:41 Changed 4 years ago by ncohen

Volker, you work on a version of Sage that exist only on your computer. Can you say where the conflict comes from ?

Nathann

comment:42 follow-up: Changed 4 years ago by vbraun

If you don't know then just wait for beta2

comment:43 in reply to: ↑ 42 Changed 4 years ago by ncohen

If you don't know then just wait for beta2

Then please release this beta2, beacuse you do not achieve anything by setting this patch in needs_work while not giving me any way to fix it. As you can see by clicking on the "commits" link, this patch has an history of around ~50 commits that are only linked together on your own computer. And it is rather hard to work with this amount of dependencies "in the dark".

Nathann

comment:44 Changed 4 years ago by git

  • Commit changed from cc0e804efc31f83e5be7fcc694416ae4a78114e7 to d7129d6e80c8d3eb11d09e2cd620882a14d6e73d

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

c207f01trac #16879: OA caching in C
80e0ceftrac #16879: orthogonal_array_recursive.py -> pyx
0549110trac #16879: a is_available function in orthogonal_arrays_recursive
0201bcbtrac #16879: more speed up
b110018trac #16879: rename orthogonal_arrays_recursive to orthogonal_arrays_find_recursive
d9e0475trac #16879: Move constructions from orthogonal_arrays_find to orthogonal_arrays_build (this, and only this)
5031aeetrac #16879: Fix the import statements
0c1893ftrac #16879: Fix the doc
6a3869dtrac #16879: speed up
d7129d6trac #16879: Trivial stuff

comment:45 Changed 4 years ago by ncohen

  • Status changed from needs_work to positive_review

Rebased !

Nathann

comment:46 Changed 4 years ago by vbraun

  • Branch changed from public/16879 to d7129d6e80c8d3eb11d09e2cd620882a14d6e73d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.