Move 'import' in orthogonal_array()

[nathanncohen]

Win some perf with a couple of improvements. Nothing fantastic, but still worth taking.

Nathann

Before

sage: %time MOLS_table(30,1)
...
CPU times: user 10.7 s, sys: 0 ns, total: 10.7 s Wall time: 10.7 s

After

sage: %time MOLS_table(30,1)
...
CPU times: user 6.98 s, sys: 4 ms, total: 6.99 s Wall time: 6.99 s

Depends on #16870

CC: @videlec

Component: combinatorial designs

Author: Vincent Delecroix

Branch/Commit: 1b36757

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/16875

[nathanncohen]

Dependencies: #16870

[nathanncohen]

Branch: public/1687

[nathanncohen]

Commit: d5562b5

[nathanncohen]

Changed branch from public/1687 to public/16875

[nathanncohen]

Last 10 new commits:

f9e6569	trac #16817: docfix
c92e9bf	trac #16846: a difference_matrices module
fdab06e	trac #16846: Remove obsolete functions
f7afe6f	trac #16846: Broken doctests
fd1bbc6	trac #16846: Merge with updated #16817
c66c19b	trac #16846: review
361c403	trac #16868: A real difference matrix has k columns
ab25059	trac #16870: Use float("inf") instead of Infinity
583622d	trac #16870: remove an import
d5562b5	trac #16875: Some caching in orthogonal_array_recursive

[sagetrac-git]

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

c40c83f	trac #16875: move imports
3c2c442	trac #16875: set the cache when we call difference_matrix

[sagetrac-git]

Changed commit from d5562b5 to 3c2c442

[videlec]

comment:4

Hello,

What we win with my first commit is impressive!

About your commit: I do not like the fact that we cache twice the same data. _OA_cache already contains everything.

Vincent

[nathanncohen]

comment:6

Yo !

What we win with my first commit is impressive!

The imports.... T_T

And they don't even appear in %prun T_T

About your commit: I do not like the fact that we cache twice the same data. _OA_cache already contains everything.

Well, do you like the timings ? Actually, it seems that your removing the imports was an important part of why the recursive functions were so slow. With your modification my caching only saves one second over the 10 that it takes to build the table up to 1000, but well... I think it's worth keeping anyway. And perhaps we will remove that when everything will be in Cython for it will not save anything anymore.

Nathann

[videlec]

comment:7

Another proposition at public/16875-bis. I implemented a orthogonal_array_available and hence avoid the double caching. I really do not like the fact that we cache twice the same information.

Vincent

[nathanncohen]

comment:8

Yo !

You may not like this additional caching (1Mb or RAM ? Yes I understand, that can be a problem) but what I do not like are public functions like yours with "the hack I need for my own code" that checks consecutive values. You just don't write that in a user-exposed function.

Here is a proposition: your two commits here are good and are useful, se we need them now. If this Mb of caching is a problem for you then we can remove it from this ticket and let it go like that.

Your function orthogonal_array_available looks a lot like the function is_available(k,n_ I proposed to implement earlier in a "designs.orthogonal_arrays" orbject, along with build(k,n), best_k(n), etc... So if we have to do something like that we will do it properly with such functions.

I will also use the chance to convert the caching system to pure Cython, so that it will become a simple array lookup, nothing more complicated.

We can also keep this caching thing in the meantime, it's up to you.

Tell me and I will start writing the code.

Nathann

[videlec]

comment:9

Hello,

You may not like this additional caching (1Mb or RAM ? Yes I understand, that can be a problem) but what I do not like are public functions like yours with "the hack I need for my own code" that checks consecutive values. You just don't write that in a user-exposed function.

Because your caching is not "the hack I need for my own code"? Moreover, in my orthogonal_array_available it makes not a big difference to remove the mult parameter. And if you prefer, I can set the name to _orthogonal_array_available. My problem is not the fact that you use memory, but the fact that you cache twice the same data.

The simplest would be to let this ticket go without _cache_m_zero_one_two and without orthogonal_array_available. And it is clear that the good solution would be to implement the Cython version of the function you suggest: is_available.

Vincent

[nathanncohen]

Changed author from Nathann Cohen to Vincent Delecroix

[nathanncohen]

comment:10

Yo !

Because your caching is not "the hack I need for my own code"?

My function is cached and is named _cache_m_zero_k_m. It SCREAMS that it is not meant for users. Yours is named "orthogonal_arrayy_available" and supports a weird parameters aboit consecutive values.

Moreover, in my orthogonal_array_available it makes not a big difference to remove the mult parameter. And if you prefer, I can set the name to _orthogonal_array_available. My problem is not the fact that you use memory, but the fact that you cache twice the same data.

You have nothing when the data being stored is 99% of useless pointers because it's dictionaries toward Integer objects, or when you store numbers between 0 and 1000 ont 64-bits integers, but it becomes a problem when instead of the useless data you store twice the same thing ? :-P

The simplest would be to let this ticket go without _cache_m_zero_one_two and without orthogonal_array_available. And it is clear that the good solution would be to implement the Cython version of the function you suggest: is_available.

Okay ! Then I update this branch in a second and give it a positive review (all the code is yours). And I will begin to write this is_availabe function in a second, everything in Cython. Let's hope that it will make a big difference !

Nathann

[nathanncohen]

Reviewer: Nathann Cohen

[sagetrac-git]

Changed commit from 3c2c442 to 1b36757

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

a64c967	trac #16875: move imports
1b36757	trac #16875: set the cache when we call difference_matrix

[videlec]

comment:12

(set back to positive review as a push changes the status)

[nathanncohen]

comment:13

Thanks !

Nathann

[vbraun]

Changed branch from public/16875 to 1b36757