is_projective_plane for incidence structure

[sagetrac-q-honore]

Add new method ìs_projective_plane to the class IncidenceStructure

CC: @videlec @nathanncohen

Component: combinatorial designs

Author: Quentin Honoré

Branch/Commit: e8c9895

Reviewer: Vincent Delecroix

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

[sagetrac-q-honore]

Commit: 0824ca6

[sagetrac-q-honore]

New commits:

0824ca6

Add is_projective_plane to incidence_structure

[sagetrac-q-honore]

Branch: u/q.honore/is_projective_plane

[videlec]

Reviewer: Vincent Delecroix

[videlec]

comment:2

Hi Quentin,

1. there is no need to mention the first argument ``s`` in the INPUT section. Moreover, as you can see this argument is often denoted self in other methods. That way it is clear that it is not really an argument.

2. In the verbose output

The number of points must be k^2 + k + 1 =6, got 7

it would be better to also have a space after the k^2 + k + 1, in other words

The number of points must be k^2 + k + 1 = 6, got 7

3. Why did you indent your comments (i.e. the lines starting with #)? If you look at other methods like __contains__ or degree you will see that there is no indentation.

4. I think you would better move your method down in the file. At least below the comment at line 1279

    #####################
    # real computations #
    #####################

As you can see, the file incidence_structures.py is rather long and it is important to keep it organized as much as possible.

5. It would be nice to add an example with labels which are not integers

    sage: B = ['abc', 'adf', 'age', 'bde', 'bgf', 'cgd', 'cef']
    sage: IncidenceStructure(B).is_projective_plane()
    True
    sage: B[-1] = 'cgf'
    sage: B[-2] = 'ced'
    sage: IncidenceStructure(B).is_projective_plane(verbose=True)
    More than one line through d and e
    False

that way you test that your error message explicitely mentions the points (and not their internal indices).

6. I am not exactly sure but it might be that the method is_t_design already implements this. The parameters would be v=k^2+k+1, t=2 and lambda=1.

Vincent

[sagetrac-git]

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

9806803

Add example, considered is_t_design method, errors removed

[sagetrac-git]

Changed commit from 0824ca6 to 9806803

[nathanncohen]

comment:4

I am not sure if such a function is actually needed, but you will find that most of the job has already been done in two functions:

• sage.combinat.designs.block_designs.are_hyperplanes_in_projective_geometry_parameters
• sage.combinat.designs.designs_pyx.is_pairwise_balanced_design

Nathann

[videlec]

comment:5

Replying to @nathanncohen:

I am not sure if such a function is actually needed, but you will find that most of the job has already been done in two functions:

Might be useful to have an alias directly on incidence structures. Though, not essential.

• sage.combinat.designs.block_designs.are_hyperplanes_in_projective_geometry_parameters

This is one is pretty unrelated

• sage.combinat.designs.designs_pyx.is_pairwise_balanced_design

This one is useful but not complete (since it does not check for the existence of quadrilateral). Moreover, I found out that there is a wrong reference sage.combinat.designs.designs_pyx.is_group_divisible_design mentions sage.combinat.designs.incidence_structures.GroupDivisibleDesign which does not exists (it is sage.combinat.designs.group_divisible_designs.GroupDivisibleDesign). Would be cool to make the correction in this ticket.

Vincent

[nathanncohen]

comment:6

This one is useful but not complete (since it does not check for the existence of quadrilateral).

Neither does the code on this branch, unless you saw something I missed.

Would be cool to make the correction in this ticket.

+1

Nathann

[videlec]

comment:7

Replying to @nathanncohen:

This one is useful but not complete (since it does not check for the existence of quadrilateral).

Neither does the code on this branch, unless you saw something I missed.

this is automatic if you specify that the number of points and blocks is v=k^2+k+1 where each block has cardinality k+1. The method is convenient because it avoids to input the parameters.

Could you precise what you think:

• there is no need for is_projective_plane
• is_projective_plane should call the existing is_group_divisible_design
• is_projective_plane would be better as a function in designs_pyx.pyx

Vincent

[nathanncohen]

comment:8

Could you precise what you think:

• there is no need for is_projective_plane
• is_projective_plane should call the existing is_group_divisible_design
• is_projective_plane would be better as a function in designs_pyx.pyx

What I see in this branch is mostly error messages, which have already been implemented in is_pairwise_balanced_design. Similarly, you check that every pair of vertices appears once, and this is already done in is_pairwise_balanced_design. In both cases, I do not see any reason to have them twice, especially when the second is faster.

If all this function does is call is_pairwise_balanced_design, then it can stay in a .py file, as it will not compute much.

Nathann

[sagetrac-git]

Changed commit from 9806803 to 6e9489d

[sagetrac-git]

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

6e9489d

Changed the wrong reference to GroupDivisibleDesign

[sagetrac-git]

Changed commit from 6e9489d to e8c9895

[sagetrac-git]

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

e8c9895

Add is_projective_plane to designs_pyx

[sagetrac-q-honore]

comment:11

Hello,
Now is_projective_plane is a function in designs_pyx.pyx. I also rebased on the last development version.

[nathanncohen]

comment:13

length of liness?

[nathanncohen]

comment:14

also, I do not get exactly why you did not use is_pairwise_balanced_design directly, since it fills the groups for you.

Nathann

[videlec]

comment:15

Replying to @nathanncohen:

length of liness?

??

[nathanncohen]

comment:16

You know, the old 80chr width stuff.

[vbraun]

Changed branch from u/q.honore/is_projective_plane to e8c9895