Opened 7 years ago
Closed 7 years ago
#16460 closed enhancement (fixed)
a cache for OA/TD/MOLS existence
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.3 |
Component: | combinatorial designs | Keywords: | |
Cc: | vdelecroix | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Vincent Delecroix |
Report Upstream: | N/A | Work issues: | |
Branch: | d7cb52a (Commits, GitHub, GitLab) | Commit: | d7cb52a2eff7c3683f1557a66a5f06fa0c5225fb |
Dependencies: | #16356 | Stopgaps: |
Description
Here it is ! Short and efficient, but OA-specific.
Right now it is not linked wth any constructor, it will be done while #16347 is implemented.
The same thing will have to be done later for incomplete_orthogonal_array
.
Nathann
Change History (13)
comment:1 Changed 7 years ago by
- Branch set to u/ncohen/16460
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Commit set to f23d39c13647376fcb12f2c1825a3053af976174
comment:3 follow-up: ↓ 4 Changed 7 years ago by
Hi Nathann,
I implemented the changes associated to the following comments in u/vdelecroix/16460.
- the default for
min_false
should ben+2
? No? - with the current implementation, the
_set_OA_cache
set a new value to_OA_cache[n]
even if nothing has changed. This is bad from the global warming point of view. - it would be better that the keys and values are only Python
int
or only SageInteger
but not a mix.
On the other hand, wouldn't it be better to have this in designs_pyx.pyx
?
Vincent
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 7 years ago by
Yo !
- the default for
min_false
should ben+2
? No?
Aahahah right. I had thought about this afterwards and forgot to change it, but I had comforted myself before by figuring out that it had actually no effect on the answers. Because "True" is tested before "False" :-)
- with the current implementation, the
_set_OA_cache
set a new value to_OA_cache[n]
even if nothing has changed. This is bad from the global warming point of view.
And you think that your added "if" have a zero cost ? To me it is rather useless and adds lines for no point...
- it would be better that the keys and values are only Python
int
or only SageInteger
but not a mix.
I'm not sensible to these problems :-P
On the other hand, wouldn't it be better to have this in
designs_pyx.pyx
?
Ahahah. We are not there yet. There is a long road in front of us before that this caching mechanism represent a non-negligible part of the computations :-P
Okay... Well, if you insist on having this "best" test I guess it can go although really I don't see the point... Otherwise if you are okay with not including it I would be glad to add to my branch the part of your commit that converts k,n to Python 'int' if it makes you sleep easier.
Nathann
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to ncohen:
Yo !
- the default for
min_false
should ben+2
? No?Aahahah right. I had thought about this afterwards and forgot to change it, but I had comforted myself before by figuring out that it had actually no effect on the answers. Because "True" is tested before "False"
:-)
- with the current implementation, the
_set_OA_cache
set a new value to_OA_cache[n]
even if nothing has changed. This is bad from the global warming point of view.And you think that your added "if" have a zero cost ? To me it is rather useless and adds lines for no point...
- it would be better that the keys and values are only Python
int
or only SageInteger
but not a mix.I'm not sensible to these problems
:-P
On the other hand, wouldn't it be better to have this in
designs_pyx.pyx
?Ahahah. We are not there yet. There is a long road in front of us before that this caching mechanism represent a non-negligible part of the computations
:-P
Okay... Well, if you insist on having this "best" test I guess it can go although really I don't see the point... Otherwise if you are okay with not including it I would be glad to add to my branch the part of your commit that converts k,n to Python 'int' if it makes you sleep easier.
Please convert to int and set the default False
to n+2
. Otherwise, you assume that there is no projective plane except for prime powers.
comment:6 Changed 7 years ago by
- Commit changed from f23d39c13647376fcb12f2c1825a3053af976174 to 60b717f372f4a377f946dea21824c86454d2b678
Branch pushed to git repo; I updated commit sha1. New commits:
60b717f | trac #16460: small fixes
|
comment:7 Changed 7 years ago by
Done !
Nathann
comment:8 Changed 7 years ago by
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to positive_review
I set to positive review. But anyway, it will be interesting only when it will be integrated in the code.
Vincent
comment:9 Changed 7 years ago by
- Dependencies set to #16295
comment:10 Changed 7 years ago by
- Dependencies changed from #16295 to #16356
comment:11 Changed 7 years ago by
- Commit changed from 60b717f372f4a377f946dea21824c86454d2b678 to d7cb52a2eff7c3683f1557a66a5f06fa0c5225fb
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:
f1c7fa1 | trac #16295: Typo
|
451d359 | trac #16295: Merged with 6.3.beta2
|
31e84b8 | trac #16295: todo note
|
6fb1f14 | trac #16295: Bugfix
|
1653790 | trac #16295: Merged with 6.3.beta3
|
b66a187 | fixed some small language typos and changed MOLS error messages into context that MOLS researchers will expect.
|
02f1247 | trac #16295: one more doctest
|
f969003 | trac #16356: MOLS for n=18,57,154,276,298,342
|
917dd5c | trac #16356: Merged with #16295
|
d7cb52a | trac #16460: Merged with updated #16356
|
comment:12 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:13 Changed 7 years ago by
- Branch changed from u/ncohen/16460 to d7cb52a2eff7c3683f1557a66a5f06fa0c5225fb
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #16460: a cache for OA/TD/MOLS existence