Opened 7 years ago
Closed 7 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, GitHub, GitLab) | Commit: | 10436405b88a88dd147ca5283a61c31f0c475aee |
Dependencies: | Stopgaps: |
Description (last modified by )
Add a function in sage.combinat.designs.block_design
to build Hughes planes.
Change History (28)
comment:1 Changed 7 years ago by
- Commit set to 41ed54d014a84a5aaf1925a99819e49ac4451af6
comment:2 Changed 7 years ago by
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 7 years ago by
Some more comments:
- 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.
- you should align the comments
# xyz
with the code
- 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.
- For the function
normalize
it would be better to say that it is only intended to be used inHughesPlane
. And you can even call itnormalize_hughes_plane_point
, in other words choose something more explicit.
- 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 7 years ago by
- Commit changed from 41ed54d014a84a5aaf1925a99819e49ac4451af6 to dce0e829bb4e93878e430cf4cf00ce88c09f05d3
comment:5 Changed 7 years ago by
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 7 years ago by
- Commit changed from dce0e829bb4e93878e430cf4cf00ce88c09f05d3 to 5a7832b0479c1400579b7b62271f88d4b6351998
comment:7 follow-up: ↓ 9 Changed 7 years ago by
- 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 7 years ago by
- 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 7 years ago by
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 7 years ago by
- Status changed from new to needs_review
comment:11 Changed 7 years ago by
- 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 uncomprehensibleFor 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 7 years ago by
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 7 years ago by
- 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 7 years ago by
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 nota=b
. Similarly, add spaces in function declarationf(a, b)
and notf(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
andn
it would be better to useq2
andq
(and do not forget to modify theINPUT
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 7 years ago by
- Commit changed from 799f921acccacc4e48dcd11b46caab9b2d0d4ea7 to 62ff365cc31c5463000fbac1db06b39efaf49f92
Branch pushed to git repo; I updated commit sha1. New commits:
62ff365 | Corrected HughesPlane description
|
comment:16 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:17 Changed 7 years ago by
- 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 7 years ago by
- Commit changed from 26056949273df618ce8ef35eb648927971f1c5a5 to 7e2070f578c112043ed8cf9d367a5ab4e20cbdff
comment:19 follow-up: ↓ 20 Changed 7 years ago by
- 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 7 years ago by
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 7 years ago by
- 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 7 years ago by
- Status changed from needs_work to needs_review
comment:23 Changed 7 years ago by
- Commit changed from 037017c98a0ce4a983fb1429ea97d7ec3907d6f7 to c398bbbbb35f8d019b70cc35cdd2a21345bd213d
comment:24 Changed 7 years ago by
- 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 7 years ago by
- 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 7 years ago by
- Status changed from needs_work to needs_review
comment:27 Changed 7 years ago by
- 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 7 years ago by
- Branch changed from public/18527 to 10436405b88a88dd147ca5283a61c31f0c475aee
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Add is_projective_plane to incidence_structure
Add example, considered is_t_design method, errors removed
Changed the wrong reference to GroupDivisibleDesign
Add functions to build Hughes planes