Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Covering Arrays #35008

Merged
merged 71 commits into from
Oct 8, 2023
Merged

Conversation

aadwyer
Copy link
Contributor

@aadwyer aadwyer commented Feb 7, 2023

Fixes #34279

aadwyer and others added 15 commits August 8, 2022 13:24
with a few initial methods: is_covering_array and the init method,
as well as that give its size n and k, its parameters t and v,
and an ordering based on the lexicographic ordering of each row.
with a few initial methods: is_covering_array and the init method,
as well as that give its size n and k, its parameters t and v,
and an ordering based on the lexicographic ordering of each row.
use of specific symbol sets for CAs  instead of just their size.
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Patch coverage: 98.31% and project coverage change: +0.01 🎉

Comparison is base (52a81cb) 88.57% compared to head (15f7fbb) 88.58%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35008      +/-   ##
===========================================
+ Coverage    88.57%   88.58%   +0.01%     
===========================================
  Files         2140     2141       +1     
  Lines       397273   397478     +205     
===========================================
+ Hits        351891   352124     +233     
+ Misses       45382    45354      -28     
Impacted Files Coverage Δ
src/sage/schemes/elliptic_curves/ell_generic.py 93.25% <66.66%> (+0.01%) ⬆️
src/sage/interfaces/tachyon.py 87.93% <90.00%> (+0.43%) ⬆️
src/sage/schemes/elliptic_curves/gal_reps.py 82.23% <90.00%> (+0.04%) ⬆️
src/sage/quadratic_forms/quadratic_form.py 90.26% <95.65%> (+0.16%) ⬆️
src/sage/combinat/designs/covering_array.py 100.00% <100.00%> (ø)
src/sage/combinat/designs/design_catalog.py 100.00% <100.00%> (ø)
src/sage/modular/quasimodform/element.py 99.20% <100.00%> (+0.06%) ⬆️
src/sage/rings/qqbar.py 95.30% <100.00%> (+<0.01%) ⬆️
src/sage/schemes/affine/affine_morphism.py 90.33% <100.00%> (ø)
src/sage/schemes/elliptic_curves/BSD.py 43.75% <100.00%> (+0.21%) ⬆️
... and 42 more

... and 32 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aadwyer aadwyer marked this pull request as ready for review March 2, 2023 01:23
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look at it tomorrow

@aadwyer
Copy link
Contributor Author

aadwyer commented Jul 25, 2023

Sorry this comment was very confusing. Covering and orthogonal arrays are looking at ordered t-subsets of columns. It is t! rather than (kt) that we are looking at. Nevertheless, one can implement two small optimizations

  • smarter iteration there is no need to run through all rows of the array every time. As soon as you saw every permutation of t elements you can stop
  • smarter stop if you succeeded in finding all permutations in a given choice of t columns, you can succeed for t+1 only if the multiplicity of each permutation is at least v (because each permutation needs to be extended by all possible symbols when adding an extra column) .

To make the second change I could change the function back to using tuple_dictionary and then check its values to ensure they all have multiplicity >v before iterating. Do you think this is more or less efficient than using existing_combinations and checking its size, but missing on this smarter iteration?

@aadwyer
Copy link
Contributor Author

aadwyer commented Jul 25, 2023

I will add examples to the covering_array.py file in my next merge but the rest of the smaller suggestions have been implemented.

@videlec
Copy link
Contributor

videlec commented Jul 26, 2023

Sorry this comment was very confusing. Covering and orthogonal arrays are looking at ordered t-subsets of columns. It is t! rather than (kt) that we are looking at. Nevertheless, one can implement two small optimizations

  • smarter iteration there is no need to run through all rows of the array every time. As soon as you saw every permutation of t elements you can stop
  • smarter stop if you succeeded in finding all permutations in a given choice of t columns, you can succeed for t+1 only if the multiplicity of each permutation is at least v (because each permutation needs to be extended by all possible symbols when adding an extra column) .

To make the second change I could change the function back to using tuple_dictionary and then check its values to ensure they all have multiplicity >v before iterating. Do you think this is more or less efficient than using existing_combinations and checking its size, but missing on this smarter iteration?

set and dictionary are very similar data structures in Python (hash tables). It should not slow down much. You can certainly cook up bad examples where the "bad" subset of columns are the t-last. In such example, the current version would run through $\binomial{k}{t} - 1$ sets of columns successfully and fail on the last one. Whereas the "smarter iteration" version would have stopped at $t-1$. But this is probably not a typical example of what you might want to test.

@videlec
Copy link
Contributor

videlec commented Aug 6, 2023

There seems to be a problem with the documentation

[sagemath_doc_html-none] OSError: /sage/src/sage/combinat/designs/covering_array.py:docstring of sage.combinat.designs.covering_array:14: WARNING: citation not found: Sher2006

(see CI report)

@github-actions
Copy link

Documentation preview for this PR (built with commit 74c1b34; changes) is ready! 🎉

@aadwyer
Copy link
Contributor Author

aadwyer commented Oct 1, 2023

Although the checks are still not successful, it seems to me that the errors come from the develop branch.

That being said, is there anything else that must be changed about this pull request for it to be accepted?
Also, will we have to wait for the checks in develop to all be successful, or are pull requests being merged anyway?

@aadwyer aadwyer requested a review from videlec October 1, 2023 21:51
@dimpase
Copy link
Member

dimpase commented Oct 2, 2023

our CI, and the latest beta, are in a mess right now, due to a not too well-tested switch to Cython 3.

@brettpim
Copy link
Contributor

brettpim commented Oct 2, 2023

DIma,
Will we have have to wait for the CI/beta to get fixed before this PR can be approved for merging? Can it be approved to it automatically merges when everything is resolved?

@dimpase
Copy link
Member

dimpase commented Oct 2, 2023

as soon as I have the latest beta working locally, I'll have a look

@vbraun vbraun merged commit d5d7c46 into sagemath:develop Oct 8, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 8, 2023
@aadwyer aadwyer deleted the t/34279/coveringarrays branch October 11, 2023 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CoveringArrays
9 participants