Fix docstring failures in designs.balanced_incomplete_block_design with use_LJCR=True

[Ivo-Maffei]

The use of use_LJCR=True in designs.balanced_incomplete_block_design give rise to few issues.

1. The returned object has type IncidenceStructure instead of BalancedIncompleteBlockDesign
2. A ValueError exception can be raised if the parameters searched are not in the database

Example for 2:

sage: designs.balanced_incomplete_block_design(100,10,1,use_LJCR=True)
Traceback (most recent call last):
...
ValueError: no (100, 10, 2) covering design in database

In addition to the above, the docstring tests that are meant to test the use of use_LJCR=True are not actually testing the LJCR online database.

Depends on #30131

CC: @dimpase

Component: combinatorial designs

Keywords: bibd, LJCR

Author: Ivo Maffei

Branch/Commit: 39e147b

Reviewer: Dima Pasechnik, Samuel Lelièvre

Issue created by migration from https://trac.sagemath.org/ticket/30107

[Ivo-Maffei]

Commit: 196eecd

[Ivo-Maffei]

Branch: u/gh-Ivo-Maffei/bibd_LJCR_fix

[Ivo-Maffei]

comment:2

I tried looking for an entry in the LJCR database that gets used by Sage, but I can't find one.
I run the code

sage: for v in range(100):
....:     for k in range(v):
....:         D = designs.balanced_incomplete_block_design(v,k,1,use_LJCR=True, existence=True)
....:         if D == 3:
....:             print(v,k)

where I changed the balanced_incomplete_block_design function to return 3 instead of True if the LJCR database was used.

I didn't obtain any results. So I think that the LJCR database is actually never used.

On a side note, to fix the URLError I swapped the url to use http instead of https. I don't think there is a security issue in retrieving values for Sage, yet you never know how Sage might get used. Hence, I believe the issue should be fixed at the level of urllib (the python library used for the request).
I don't have the knowledge to fix it myself, though.

---

New commits:

196eecd

fixed bugs in LJCR DB use

[dimpase]

comment:3

to see the errors (before this branch), run

./sage -tp --optional=build,dochtml,memlimit,internet,sage src/sage/combinat/designs/

In particular I see

sage -t --warn-long 81.5 src/sage/combinat/designs/bibd.py
**********************************************************************
File "src/sage/combinat/designs/bibd.py", line 157, in sage.combinat.designs.bibd.balanced_incomplete_block_design
Failed example:
    B                                                              # optional - internet
Expected:
    Incidence structure with 66 points and 143 blocks
Got:
    (66,6,1)-Balanced Incomplete Block Design
**********************************************************************
File "src/sage/combinat/designs/bibd.py", line 161, in sage.combinat.designs.bibd.balanced_incomplete_block_design
Failed example:
    designs.balanced_incomplete_block_design(66, 6, 1, use_LJCR=True)  # optional - internet
Expected:
    Incidence structure with 66 points and 143 blocks
Got:
    (66,6,1)-Balanced Incomplete Block Design
**********************************************************************

[dimpase]

comment:4

With your branch, all the tests as in comment:3 pass. However, I can revert your change https->http as follows and still have all the tests pass.

--- a/src/sage/combinat/designs/covering_design.py
+++ b/src/sage/combinat/designs/covering_design.py
@@ -521,7 +521,7 @@ def best_known_covering_design_www(v, k, t, verbose=False):
     k = int(k)
     t = int(t)
     param = "?v=%s&k=%s&t=%s" % (v, k, t)
-    url = "http://ljcr.dmgordon.org/cover/get_cover.php" + param
+    url = "https://ljcr.dmgordon.org/cover/get_cover.php" + param
     if verbose:
         print("Looking up the bounds at %s" % url)
 

It appears that in your Sage's python URLlib is broken. Of course it should be able to handle https. Tell me more about your ./sage --python. Do you build it, or use one from OS?

[dimpase]

Reviewer: Dima Pasechnik

[dimpase]

comment:5

Is this otherwise ready for review?

[Ivo-Maffei]

comment:6

I'm not sure where the python software I have comes from.
If it can help I have the following output

% sage --python
Python 3.7.3 (default, May  6 2020, 12:19:52) 
[Clang 11.0.3 (clang-1103.0.32.59)] on darwin
Type "help", "copyright", "credits" or "license" for more information.

Actually the full traceback when I get the URLError is the following:

URLError                                  Traceback (most recent call last)
<ipython-input-1-aace303dd913> in <module>()
----> 1 designs.balanced_incomplete_block_design(Integer(85),Integer(7),Integer(1),use_LJCR=True)

/Users/ivomaffei/sage/local/lib/python3.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3675)()
    351             True
    352         """
--> 353         return self.get_object()(*args, **kwds)
    354 
    355     def __repr__(self):

/Users/ivomaffei/sage/local/lib/python3.7/site-packages/sage/combinat/designs/bibd.py in balanced_incomplete_block_design(v, k, lambd, existence, use_LJCR)
    253         values_in_db = False
    254         try:
--> 255             B = best_known_covering_design_www(v, k, 2)
    256             values_in_db = True
    257         except ValueError:

/Users/ivomaffei/sage/local/lib/python3.7/site-packages/sage/combinat/designs/covering_design.py in best_known_covering_design_www(v, k, t, verbose)
    526         print("Looking up the bounds at %s" % url)
    527 
--> 528     f = urlopen(url)
    529     try:
    530         s = bytes_to_str(f.read())

/Users/ivomaffei/sage/local/lib/python3.7/urllib/request.py in urlopen(url, data, timeout, cafile, capath, cadefault, context)
    220     else:
    221         opener = _opener
--> 222     return opener.open(url, data, timeout)
    223 
    224 def install_opener(opener):

/Users/ivomaffei/sage/local/lib/python3.7/urllib/request.py in open(self, fullurl, data, timeout)
    523             req = meth(req)
    524 
--> 525         response = self._open(req, data)
    526 
    527         # post-process response

/Users/ivomaffei/sage/local/lib/python3.7/urllib/request.py in _open(self, req, data)
    546 
    547         return self._call_chain(self.handle_open, 'unknown',
--> 548                                 'unknown_open', req)
    549 
    550     def error(self, proto, *args):

/Users/ivomaffei/sage/local/lib/python3.7/urllib/request.py in _call_chain(self, chain, kind, meth_name, *args)
    501         for handler in handlers:
    502             func = getattr(handler, meth_name)
--> 503             result = func(*args)
    504             if result is not None:
    505                 return result

/Users/ivomaffei/sage/local/lib/python3.7/urllib/request.py in unknown_open(self, req)
   1385     def unknown_open(self, req):
   1386         type = req.type
-> 1387         raise URLError('unknown url type: %s' % type)
   1388 
   1389 def parse_keqv_list(l):

URLError: <urlopen error unknown url type: https>

So it seems the issues lies in the python shipped with sage.

Apart from the above, I would like to substitute the parameters of the docstring test as at the moment they are not testing the LJCR database. However, I can't find any set of parameters for which the LJCR database is actually used by sage.

[slel]

comment:7

Seems you built or installed Sage without OpenSSL.

[Ivo-Maffei]

comment:8

I use macOS which ships with LibreSSL and sort of "redirect" all OpenSSL commands to LibreSSL, i.e.

% openssl version
LibreSSL 2.8.3

I looked back at the installation guide (https://doc.sagemath.org/html/en/installation/source.html#prerequisites) and it seems that OpenSSL is recommended but not strictly required.
I personally, would like to avoid the need to install OpenSSL systemwide.

If OpenSSL is actually required (as it seems in this case), then the installation guide should be changed.
Moreover, reading both the "Install from Source Code" and "Install from Pre-built Binaries" sections I have another doubt:
the former states that it is not possible to distribute OpenSSL with Sage, yet the latter doesn't mention OpenSSL under the macOS section, so that section would also need to change.

[dimpase]

comment:9

We have had long discussions on OpenSSL, you can check sage-devel...

Well, thanks to Apple for breaking Python on macOS, what can I say.

You can install Homebrew Python3, so it will live in /usr/local,
or use something like docker to test on unbroken Python, or build Sage's Python
by configuring with --with-system-python=no (and then you'd also need to first install Sage's openssl and pyopenssl packages)

Anyhow, just revert the change https->http, so that I can set this ticket to positive review.

[sagetrac-git]

Changed commit from 196eecd to 836d9b9

[sagetrac-git]

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

836d9b9

revert http to https

[dimpase]

comment:12

lgtm

[slel]

comment:13

Some more suggestions if you care. Otherwise just set back to positive review.

Remove this commented-out print (debugging leftover?)

        #print("LJCR with params {}, {}, 1".format(v,k))

Remove space before question mark:

-            # Is it a BIBD or just a good covering ?
+            # Is it a BIBD or just a good covering?

Add space around //:

-            expected_n_of_blocks = binomial(v, 2)//binomial(k, 2)
+            expected_n_of_blocks = binomial(v, 2) // binomial(k, 2)

Use f-strings and lowercase.

-                raise EmptySetError("There exists no ({},{},{})-BIBD".format(v, k, lambd))
+                raise EmptySetError(f"there exists no ({v},{k},{lambd})-BIBD")

[slel]

comment:14

After the class BalancedIncompleteBlockDesign,
an alias BIBD = BalancedIncompleteBlockDesign
could help shorten many long lines.

[sagetrac-git]

Changed commit from 836d9b9 to 59f43c1

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

41eb3ec	fixed bugs in LJCR DB use
89fac3f	revert http to https
59f43c1	code formatting

[Ivo-Maffei]

comment:16

I fixed the suggested formatting and rebased the branch on 9.2.beta5

[slel]

comment:17

Looks good to me if the patchbot is happy.

[slel]

Changed reviewer from Dima Pasechnik to Dima Pasechnik, Samuel Lelièvre

[slel]

comment:18

Regarding OpenSSL for Sage on macOS, you can fix
your Sage installation using fix_mac_sage.

• https://github.com/3-manifolds/fix_mac_sage/

[slel]

comment:19

Bots are happy. Positive review.

Regarding the minor warnings:

• Pycodestyle complains about "multiple statements on one line".
  Those are not from this ticket, so they could be dealt with elsewhere.
• Pyflakes complains about the f-string, maybe it still wants py2 and py3 compatible code.

[slel]

comment:20

The "multiple statements on one line" are fixed at #30131.

[sagetrac-git]

Changed commit from 59f43c1 to e4b173f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

8cf4741	fix optional internet doctests in bibd
e4b173f	merged #30131 and fixed conflict

[Ivo-Maffei]

Dependencies: 30131

[Ivo-Maffei]

Changed dependencies from 30131 to #30131

[Ivo-Maffei]

comment:23

I merged the tickets because there was a merge conflict

[dimpase]

comment:24

coudln't

+    elif x <= 66:
+        t,u = 30*s+11, x-55
+    elif x <= 96:
+        t,u = 30*s+11, x-55
+    elif x <= 121:
+        t,u = 30*s+11, x-55

be simplified to

+    elif x <= 121:
+        t,u = 30*s+11, x-55

?

[sagetrac-git]

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

39e147b

simplified if-else statement

[sagetrac-git]

Changed commit from e4b173f to 39e147b

[dimpase]

comment:26

OK, good

[vbraun]

Changed branch from u/gh-Ivo-Maffei/bibd_LJCR_fix to 39e147b