Opened 2 years ago

Closed 2 years ago

#18527 closed enhancement (fixed)

Hughes Plane (combinatorial design)

Reported by: q.honore Owned by:
Priority: major Milestone: sage-6.8
Component: combinatorial designs Keywords:
Cc: vdelecroix, ncohen Merged in:
Authors: Quentin Honoré Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 1043640 (Commits) Commit: 10436405b88a88dd147ca5283a61c31f0c475aee
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

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

Change History (28)

comment:1 Changed 2 years ago by git

  • Commit set to 41ed54d014a84a5aaf1925a99819e49ac4451af6

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

0824ca6Add is_projective_plane to incidence_structure
9806803Add example, considered is_t_design method, errors removed
6e9489dChanged the wrong reference to GroupDivisibleDesign
41ed54dAdd functions to build Hughes planes

comment:2 Changed 2 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 2 years ago by vdelecroix

Some more comments:

  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 2 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:

e8c9895Add is_projective_plane to designs_pyx
4e4e4a4Add functions to build Hughes planes
e4b099aAdd HughesPlane in the catalog
dce0e82Changed normalize function name, add descriptions and examples

comment:5 Changed 2 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 2 years ago by git

  • Commit changed from dce0e829bb4e93878e430cf4cf00ce88c09f05d3 to 5a7832b0479c1400579b7b62271f88d4b6351998

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

3ca2bb8Changed the HughesPlane description (\leq, not \subseteq)
5a7832bChanged BlockDesign to BIBD for HughesPlane and DesarguesianProjectivePlaneDesign

comment:7 follow-up: Changed 2 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 item had failures:
   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 2 years ago by git

  • Commit changed from 5a7832b0479c1400579b7b62271f88d4b6351998 to 2182aaad7de85fc5d35aecac64996fc14f95539a

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

2182aaaExample corrected in incidence_structure.py

comment:9 in reply to: ↑ 7 Changed 2 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 2 years ago by q.honore

  • Status changed from new to needs_review

comment:11 Changed 2 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:

47c7c21Trac #18527 (review): improve the documentation

comment:12 Changed 2 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 2 years ago by git

  • Commit changed from 47c7c21ee183abe487b7376a7bc38834f7b2177c to 799f921acccacc4e48dcd11b46caab9b2d0d4ea7

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

799f921Changed HughesPlane description according to the review

comment:14 Changed 2 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.

Some more comments:

  • 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 2 years ago by git

  • Commit changed from 799f921acccacc4e48dcd11b46caab9b2d0d4ea7 to 62ff365cc31c5463000fbac1db06b39efaf49f92

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

62ff365Corrected HughesPlane description

comment:16 Changed 2 years ago by q.honore

  • Status changed from needs_work to needs_review

comment:17 Changed 2 years ago by git

  • Commit changed from 62ff365cc31c5463000fbac1db06b39efaf49f92 to 26056949273df618ce8ef35eb648927971f1c5a5

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

2605694Trac #18527: (review) remove trailing whitespaces

comment:18 Changed 2 years ago by git

  • Commit changed from 26056949273df618ce8ef35eb648927971f1c5a5 to 7e2070f578c112043ed8cf9d367a5ab4e20cbdff

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

8cd2b5bTrac #18527: (review) rename random_q3_xxx -> q3_xxx
a07874fTrac #18257: (review) better q3_minus_one_matrix
bd692eaTrac #18527: (review) fix the case of non prime q
7e2070fTrac #18527: (review) small cleanup

comment:19 follow-up: Changed 2 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 2 years ago by vdelecroix

Replying to 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>

Oups. I understood... you are not testing the same thing! Sorry. Forget about this specific comment.

comment:21 Changed 2 years ago by git

  • Commit changed from 7e2070f578c112043ed8cf9d367a5ab4e20cbdff to 037017c98a0ce4a983fb1429ea97d7ec3907d6f7

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

037017cAdd authors, cleaned description

comment:22 Changed 2 years ago by q.honore

  • Status changed from needs_work to needs_review

New commits:

037017cAdd authors, cleaned description

New commits:

037017cAdd authors, cleaned description

comment:23 Changed 2 years ago by git

  • Commit changed from 037017c98a0ce4a983fb1429ea97d7ec3907d6f7 to c398bbbbb35f8d019b70cc35cdd2a21345bd213d

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

e1c88aeTrac #18257: (review) remove trailing whitespaces
c398bbbTrac #18527: (review) cleaning

comment:24 Changed 2 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 2 years ago by git

  • Commit changed from c398bbbbb35f8d019b70cc35cdd2a21345bd213d to 10436405b88a88dd147ca5283a61c31f0c475aee

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

1043640Add HughesPlane to the catalog

comment:26 Changed 2 years ago by q.honore

  • Status changed from needs_work to needs_review

comment:27 Changed 2 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 2 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.