Opened 5 years ago

Closed 5 years ago

# Hughes Plane (combinatorial design)

Reported by: Owned by: q.honore major sage-6.8 combinatorial designs vdelecroix, ncohen Quentin Honoré Vincent Delecroix N/A 1043640 (Commits) 10436405b88a88dd147ca5283a61c31f0c475aee

Add a function in sage.combinat.designs.block_design to build Hughes planes.

### comment:1 Changed 5 years ago by git

• Commit set to 41ed54d014a84a5aaf1925a99819e49ac4451af6

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

 ​0824ca6 Add is_projective_plane to incidence_structure ​9806803 Add example, considered is_t_design method, errors removed ​6e9489d Changed the wrong reference to GroupDivisibleDesign ​41ed54d Add functions to build Hughes planes

### comment:2 Changed 5 years ago by vdelecroix

Hello Quentin,

You need to start a new branch for this ticket. Here you started from the same as for #18439.

Vincent

### comment:3 Changed 5 years ago by vdelecroix

1. you should break the lines in your comments. The line The construction of a Hughes plane is based on a nearfield ... is one kilometer long. Try to keep the width around 80.
1. you should align the comments # xyz with the code
1. It would be cool to have some code in the EXAMPLES section which proves that Hughes plane are indeed different from the standard finite projective plane. You can show that the theorem of Desargues is not satisfied.
1. For the function normalize it would be better to say that it is only intended to be used in HughesPlane. And you can even call it normalize_hughes_plane_point, in other words choose something more explicit.
1. In the function HughesPlane, it would be nice to add few words explaining the construction and also add references (for example to Hughes original article and also Dembowski 1971)

Vincent

### comment:4 Changed 5 years ago by git

• Commit changed from 41ed54d014a84a5aaf1925a99819e49ac4451af6 to dce0e829bb4e93878e430cf4cf00ce88c09f05d3

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

 ​e8c9895 Add is_projective_plane to designs_pyx ​4e4e4a4 Add functions to build Hughes planes ​e4b099a Add HughesPlane in the catalog ​dce0e82 Changed normalize function name, add descriptions and examples

### comment:5 Changed 5 years ago by ncohen

Instead of calling 'is_projective_plane' in the constructor of desarguesian projective planes, could you make it return a BIBD instead of a Block Design? This way, the constructor would automatically check the property.

Thanks,

Nathann

### comment:6 Changed 5 years ago by git

• Commit changed from dce0e829bb4e93878e430cf4cf00ce88c09f05d3 to 5a7832b0479c1400579b7b62271f88d4b6351998

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

 ​3ca2bb8 Changed the HughesPlane description (\leq, not \subseteq) ​5a7832b Changed BlockDesign to BIBD for HughesPlane and DesarguesianProjectivePlaneDesign

### comment:7 follow-up: ↓ 9 Changed 5 years ago by vdelecroix

• Authors changed from q.honore to Quentin Honoré
• Type changed from PLEASE CHANGE to enhancement

The modification in 5a7832b is responsible for this failure:

sage -t --long combinat/designs/incidence_structures.py
**********************************************************************
File "combinat/designs/incidence_structures.py", line 1500, in sage.combinat.designs.incidence_structures.IncidenceStructure.automorphism_group
Failed example:
P = designs.DesarguesianProjectivePlaneDesign(2); P
Expected:
Incidence structure with 7 points and 7 blocks
Got:
(7,3,1)-Balanced Incomplete Block Design
**********************************************************************
1 of  12 in sage.combinat.designs.incidence_structures.IncidenceStructure.automorphism_group
[294 tests, 1 failure, 1.42 s]


When you are done with this correction, you can switch the status to needs review.

Vincent

### comment:8 Changed 5 years ago by git

• Commit changed from 5a7832b0479c1400579b7b62271f88d4b6351998 to 2182aaad7de85fc5d35aecac64996fc14f95539a

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

 ​2182aaa Example corrected in incidence_structure.py

### comment:9 in reply to: ↑ 7 Changed 5 years ago by vdelecroix

From comment:7:

When you are done with this correction, you can switch the status to needs_review.

This means: go to modify ticket in the bottom of the page. And in the Action part chose the needs_review.

### comment:10 Changed 5 years ago by q.honore

• Status changed from new to needs_review

### comment:11 Changed 5 years ago by vdelecroix

• Branch changed from u/q.honore/hughes_plane to public/18527
• Commit changed from 2182aaad7de85fc5d35aecac64996fc14f95539a to 47c7c21ee183abe487b7376a7bc38834f7b2177c
• Reviewers set to Vincent Delecroix
• Status changed from needs_review to needs_work

Hello Quentin,

There is a lot of work to do in the documentation. Most of it is uncomprehensible. Hopefully I know what you are trying to do but any other person will not get a word. I already modified some of it in my commit at public/18527 but there are still a lot of things to do:

• In HughesPlane the following sentence is uncomprehensible
    For p an odd prime.
If kernel K of order p^n coincides with the center of a nearfield N.
The construction of a Hughes plane is based on N of order p^2n.

you should explain simply what is the Hughes plane. If it is complicated just make a reference to wikipedia and just explain what it is (e.g. The Hughes plane is a finite projective plane introduced by XYZ and which is not Desarguesian)
• If you want to include mathematics formula in the documentation you must use the block MATH. This is described in details in the developer's guide
• the example that shows that the Hughes plane is not Desarguesian is not finished... And you should explain what you are doing with sentences (not Python comments starting with #). I already modified and simplified the begining.

Vincent

New commits:

 ​47c7c21 Trac #18527 (review): improve the documentation

### comment:12 Changed 5 years ago by vdelecroix

And also: always put a space around equality signs a = b and not a=b. Similarly, in functions write f(a, b, c) and not f(a,b,c).

### comment:13 Changed 5 years ago by git

• Commit changed from 47c7c21ee183abe487b7376a7bc38834f7b2177c to 799f921acccacc4e48dcd11b46caab9b2d0d4ea7

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

 ​799f921 Changed HughesPlane description according to the review

### comment:14 Changed 5 years ago by vdelecroix

Hello,

When you are done with your changes, you need to set back to needs review. That way I know that I need to have a look.

• with your last commit the test do not pass
• put spaces around equality signs a = b and not a=b. Similarly, add spaces in function declaration f(a, b) and not f(a,b).
• When you define the Hughes plane you start with p being an odd prime but it is never used
• for coherence of notations, instead of using n2 and n it would be better to use q2 and q (and do not forget to modify the INPUT section)

I did a bit of profiling and it appears that most of the time is spent in doing matrix by vector multiplication! I will try to see whether it is possible to improve that part (in another ticket).

Vincent

### comment:15 Changed 5 years ago by git

• Commit changed from 799f921acccacc4e48dcd11b46caab9b2d0d4ea7 to 62ff365cc31c5463000fbac1db06b39efaf49f92

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

 ​62ff365 Corrected HughesPlane description

### comment:16 Changed 5 years ago by q.honore

• Status changed from needs_work to needs_review

### comment:17 Changed 5 years ago by git

• Commit changed from 62ff365cc31c5463000fbac1db06b39efaf49f92 to 26056949273df618ce8ef35eb648927971f1c5a5

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

 ​2605694 Trac #18527: (review) remove trailing whitespaces

### comment:18 Changed 5 years ago by git

• Commit changed from 26056949273df618ce8ef35eb648927971f1c5a5 to 7e2070f578c112043ed8cf9d367a5ab4e20cbdff

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

 ​8cd2b5b Trac #18527: (review) rename random_q3_xxx -> q3_xxx ​a07874f Trac #18257: (review) better q3_minus_one_matrix ​bd692ea Trac #18527: (review) fix the case of non prime q ​7e2070f Trac #18527: (review) small cleanup

### comment:19 follow-up: ↓ 20 Changed 5 years ago by vdelecroix

• Status changed from needs_review to needs_work

In my review commits I changed few things (removed random from random_q3_minus_one_matrix, make HughesPlane works for non prime q and improve a bit the code).

There are still some issues with the documentation. Please pay more attention to what you write.

You wrote

Return the Hughes projective plane of order n2.


But the name of the parameter is now q2. Moreover you must use double quotes for the parameters q2. Simple ones are for latex code.

This is not a sentence

For q an odd prime.


When you test the fact that it is not Desarguesian, it is weird to do it in two ways. Here you print the name of the points

        sage: set(b_0_1).intersection(b_57_70)
{2}
sage: set(b_1_10).intersection(b_70_59)
{73}
sage: set(b_10_0).intersection(b_59_57)
{60}


But then you just test equality

        sage: p = set(b_0_57).intersection(b_1_70)
sage: q = set(b_1_70).intersection(b_10_59)
sage: p == q
False


You should do it coherently.

You can also add a section AUTHORS with your name in the function HughesPlane (see the developer guide).

Vincent

### comment:20 in reply to: ↑ 19 Changed 5 years ago by vdelecroix

When you test the fact that it is not Desarguesian, it is weird to do it in two ways. Here you print the name of the points <SNIP> But then you just test equality <SNIP>

### comment:21 Changed 5 years ago by git

• Commit changed from 7e2070f578c112043ed8cf9d367a5ab4e20cbdff to 037017c98a0ce4a983fb1429ea97d7ec3907d6f7

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

 ​037017c Add authors, cleaned description

### comment:22 Changed 5 years ago by q.honore

• Status changed from needs_work to needs_review

New commits:

 ​037017c Add authors, cleaned description

New commits:

 ​037017c Add authors, cleaned description

### comment:23 Changed 5 years ago by git

• Commit changed from 037017c98a0ce4a983fb1429ea97d7ec3907d6f7 to c398bbbbb35f8d019b70cc35cdd2a21345bd213d

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

 ​e1c88ae Trac #18257: (review) remove trailing whitespaces ​c398bbb Trac #18527: (review) cleaning

### comment:24 Changed 5 years ago by vdelecroix

• Status changed from needs_review to needs_work

When you add some stuff pay attention to the fact that you do not add trailing whitespaces (i.e. spaces at the end of the lines).

I added a small commit which move around some stuff and simplify the documentation.

There is one more thing to do: the new function HughesPlane does not appear in the list of the functions available in the top of the file design_catalog.py. You can put the reference just after DesarguesianProjectivePlaneDesign.

Vincent

### comment:25 Changed 5 years ago by git

• Commit changed from c398bbbbb35f8d019b70cc35cdd2a21345bd213d to 10436405b88a88dd147ca5283a61c31f0c475aee

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

 ​1043640 Add HughesPlane to the catalog`

### comment:26 Changed 5 years ago by q.honore

• Status changed from needs_work to needs_review

### comment:27 Changed 5 years ago by vdelecroix

• Description modified (diff)
• Status changed from needs_review to positive_review
• Summary changed from Hughes Plane for block_design to Hughes Plane (combinatorial design)

### comment:28 Changed 5 years ago by vbraun

• Branch changed from public/18527 to 10436405b88a88dd147ca5283a61c31f0c475aee
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.