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

Add skew Hadamard matrices up to order 1000 #35211

Merged

Conversation

MatteoCati
Copy link
Contributor

πŸ“š Description

This PR adds constructions for all known skew Hadamard matrices of order up to 1000. The following changes have been made:

  • more skew Supplementary difference sets have been added
  • The functions for supplementary difference sets in difference_family.py have been changed so that now they also return the group to which the sets belong. This makes the functions consistent with the others contained in this file, and makes them more efficient.
  • Added construction of skew Hadamard matrices from complementary difference sets (and construction for complementary difference sets).
  • Added construction of skew Hadamard matrices from amicable Hadamard matrices and Orthogonal Designs (and construction for some amicable Hadamard matrices).

Note that only commits from c0eb0c1 are new, the older ones where created in #35059.

πŸ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

βŒ› Dependencies

#35059: this PR adds (non skew) Hadamard matrices of order <= 1000

@dimpase
Copy link
Member

dimpase commented Mar 23, 2023

can we get a test function, with _ prefix, to check consistency: for each $0&lt; k &lt; 251$, check that a matrix of order $4k$ can be built (unless it it is one of the few exceptions we know). It can be doctested with # long time tag (unless it's too long to run)

And something similar for the skew case.

@MatteoCati
Copy link
Contributor Author

I've added the function, but I didn't write the tests because they would take too long (around 10 min each).

@github-actions
Copy link

Documentation preview for this PR is ready! πŸŽ‰
Built with commit: c4abaaf

@tscrim
Copy link
Collaborator

tscrim commented Mar 25, 2023

It still needs a doctest that is run. I would extend the functionality slightly by adding an extra k parameter for the upper bound to check (specifying that it must be less than some fixed number where we know where Sage should not be successful. It should also raise an error when it cannot construct something Sage thinks it can and print a warning when it can construct something we didn't think it could.

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

a function with prefixed by _ name does not require documentation, leave alone a doctest.

@tscrim
Copy link
Collaborator

tscrim commented Mar 25, 2023

@dimpase That is completely wrong. That only applies to cdef functions for the technical reason they cannot be directly called by Python, but it is good for even those to have an indirect test. Every function and method is required to have some form of a doctest (see the doctest coverage part of the reviewer's checklist; plus it is just good programming practice.

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

No, this is too restrictive. If true, this'd have effectively prevented Sage to have functions which are too long to run in doctests. And the function we are talking about is one of these.

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

It is effectively a test itself, to be triggered by hand. There are other such functions, e.g. in graphs/

@tscrim
Copy link
Collaborator

tscrim commented Mar 25, 2023

It is the rule, one I fully support. To me, this is suggesting that the function design is wrong. Again, see my suggestion about adding another parameter to set the upper bound. Or this should just become a doctest, with perhaps a smaller bound that is tested. Right now, this function is useless because it is not tested (so if something breaks, nobody will know) and not publicly shown (so nobody will even know to do this test).

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

It is in the source, thus it is known.
Look, we can just remove it, and then you have no reasons to complain - but then doing what it does has to be done by hand. This is ludicrous, isn't it?

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

It is an important function, just too slow, for a good reason, to be tested in a doctest. That's all.

@tscrim
Copy link
Collaborator

tscrim commented Mar 25, 2023

Being in the source code (but not in the public documentation) does not make it discoverable. Who is going to even know about this, much less test it (which you are saying needs to be done by hand)? It is not even clear what that correct output should be. If it was just a doctest itself (marked with # not tested), then it would just need to be copy/pasted by an interested party (and made available to the public).

From another perspective, all of these constructions are actually tested at some place in the code. The point is to have a way for the user to iterate over the list of constructions to see which actually work in some range. Right now, it is basically just a doctest since the bounds are fixed, but it is put as a function. It seems like you want something more like primes(a, b) that returns all prime numbers in the range [a, b]. This could then have a shorter test and would make sense to make public.

In short, if it is important, then why isn't it public (and part of the standard doctests with some cutoff)?

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

would it be OK to remove _ from the name and add a doctest tagged # not tested - too long ?

Also, make the name more descriptive, i.e. not _get_all_hadamard_matrices()
but check_hadamard_matrices_up_to_order_1000(), but

@tscrim
Copy link
Collaborator

tscrim commented Mar 25, 2023

Strictly speaking, that is not sufficient, but I can compromise on it. However, it still is speaking to me and saying that it should just be a doctest rather than a function. It feels like a magic number is being used.

@dimpase
Copy link
Member

dimpase commented Mar 25, 2023

OK, @MatteoCati - how about indeed converting it into doctests, (tagged # not tested) to be put into the header
of the file (like, for instance, is done in src/sage/numerical/mip.pyx).

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you. One last little thing (mostly formatting), but you are free to choose which option (some variation of it is also okay).

src/sage/combinat/matrices/hadamard_matrix.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you.

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.

Great!

@vbraun vbraun merged commit ac79f65 into sagemath:develop Apr 1, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
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.

None yet

7 participants