Opened 6 years ago

Closed 6 years ago

#16875 closed enhancement (fixed)

Move 'import' in orthogonal_array()

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

Description (last modified by ncohen)

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

Change History (14)

comment:1 Changed 6 years ago by ncohen

  • Branch set to public/1687
  • Dependencies set to #16870
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by ncohen

  • Branch changed from public/1687 to public/16875
  • Commit set to d5562b5583457c094480b6875189273136dd1fc2

Last 10 new commits:

f9e6569trac #16817: docfix
c92e9bftrac #16846: a difference_matrices module
fdab06etrac #16846: Remove obsolete functions
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
d5562b5trac #16875: Some caching in orthogonal_array_recursive

comment:3 Changed 6 years ago by git

  • Commit changed from d5562b5583457c094480b6875189273136dd1fc2 to 3c2c442265f4c69d05a29921ca80447edc847f28

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

c40c83ftrac #16875: move imports
3c2c442trac #16875: set the cache when we call difference_matrix

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

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

comment:5 Changed 6 years ago by vdelecroix

  • Status changed from needs_review to needs_info

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

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

comment:7 Changed 6 years ago by vdelecroix

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

comment:8 follow-up: Changed 6 years ago by ncohen

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

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by vdelecroix

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

comment:10 in reply to: ↑ 9 Changed 6 years ago by ncohen

  • Authors changed from Nathann Cohen to Vincent Delecroix
  • Reviewers set to Nathann Cohen
  • Status changed from needs_info to positive_review
  • Summary changed from Some caching in orthogonal_array_recursive to Move 'import' in orthogonal_array()

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

comment:11 Changed 6 years ago by git

  • Commit changed from 3c2c442265f4c69d05a29921ca80447edc847f28 to 1b36757e15db03b42e1e641d8929196e3f02d9b8
  • Status changed from positive_review to needs_review

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

a64c967trac #16875: move imports
1b36757trac #16875: set the cache when we call difference_matrix

comment:12 Changed 6 years ago by vdelecroix

  • Status changed from needs_review to positive_review

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

comment:13 Changed 6 years ago by ncohen

Thanks !

Nathann

comment:14 Changed 6 years ago by vbraun

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