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 )
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
- 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
- Branch changed from public/1687 to public/16875
- Commit set to d5562b5583457c094480b6875189273136dd1fc2
comment:3 Changed 6 years ago by
- Commit changed from d5562b5583457c094480b6875189273136dd1fc2 to 3c2c442265f4c69d05a29921ca80447edc847f28
comment:4 follow-up: ↓ 6 Changed 6 years ago by
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
- Status changed from needs_review to needs_info
comment:6 in reply to: ↑ 4 Changed 6 years ago by
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
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: ↓ 9 Changed 6 years ago by
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: ↓ 10 Changed 6 years ago by
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
- 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 themult
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 withoutorthogonal_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
- Commit changed from 3c2c442265f4c69d05a29921ca80447edc847f28 to 1b36757e15db03b42e1e641d8929196e3f02d9b8
- Status changed from positive_review to needs_review
comment:12 Changed 6 years ago by
- 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
Thanks !
Nathann
comment:14 Changed 6 years ago by
- Branch changed from public/16875 to 1b36757e15db03b42e1e641d8929196e3f02d9b8
- Resolution set to fixed
- Status changed from positive_review to closed
Last 10 new commits:
trac #16817: docfix
trac #16846: a difference_matrices module
trac #16846: Remove obsolete functions
trac #16846: Broken doctests
trac #16846: Merge with updated #16817
trac #16846: review
trac #16868: A real difference matrix has k columns
trac #16870: Use float("inf") instead of Infinity
trac #16870: remove an import
trac #16875: Some caching in orthogonal_array_recursive