Opened 6 years ago

Closed 5 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) 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 6 years ago by ncohen

  • Branch set to u/ncohen/16460
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by git

  • Commit set to f23d39c13647376fcb12f2c1825a3053af976174

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

f23d39ctrac #16460: a cache for OA/TD/MOLS existence

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

Hi Nathann,

I implemented the changes associated to the following comments in u/vdelecroix/16460.

  • the default for min_false should be n+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 Sage Integer 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: Changed 6 years ago by ncohen

Yo !

  • the default for min_false should be n+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 Sage Integer 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 6 years ago by vdelecroix

Replying to ncohen:

Yo !

  • the default for min_false should be n+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 Sage Integer 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 6 years ago by git

  • Commit changed from f23d39c13647376fcb12f2c1825a3053af976174 to 60b717f372f4a377f946dea21824c86454d2b678

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

60b717ftrac #16460: small fixes

comment:7 Changed 6 years ago by ncohen

Done !

Nathann

comment:8 Changed 6 years ago by vdelecroix

  • 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 6 years ago by ncohen

  • Dependencies set to #16295

comment:10 Changed 6 years ago by ncohen

  • Dependencies changed from #16295 to #16356

comment:11 Changed 6 years ago by git

  • 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:

f1c7fa1trac #16295: Typo
451d359trac #16295: Merged with 6.3.beta2
31e84b8trac #16295: todo note
6fb1f14trac #16295: Bugfix
1653790trac #16295: Merged with 6.3.beta3
b66a187fixed some small language typos and changed MOLS error messages into context that MOLS researchers will expect.
02f1247trac #16295: one more doctest
f969003trac #16356: MOLS for n=18,57,154,276,298,342
917dd5ctrac #16356: Merged with #16295
d7cb52atrac #16460: Merged with updated #16356

comment:12 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:13 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/16460 to d7cb52a2eff7c3683f1557a66a5f06fa0c5225fb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.