Opened 7 years ago
Closed 7 years ago
#18439 closed enhancement (fixed)
is_projective_plane for incidence structure
Reported by:  q.honore  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  combinatorial designs  Keywords:  
Cc:  vdelecroix, ncohen  Merged in:  
Authors:  Quentin Honoré  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  e8c9895 (Commits, GitHub, GitLab)  Commit:  e8c9895d0506eb3a68419915bf314f72a1863610 
Dependencies:  Stopgaps: 
Description
Add new method ìs_projective_plane
to the class IncidenceStructure
Change History (17)
comment:1 Changed 7 years ago by
 Branch set to u/q.honore/is_projective_plane
 Commit set to 0824ca649d08566381eb74d525744cf3c4aafeb4
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_work
Hi Quentin,
 there is no need to mention the first argument
``s``
in theINPUT
section. Moreover, as you can see this argument is often denotedself
in other methods. That way it is clear that it is not really an argument.
 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
 Why did you indent your comments (i.e. the lines starting with
#
)? If you look at other methods like__contains__
ordegree
you will see that there is no indentation.
 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 fileincidence_structures.py
is rather long and it is important to keep it organized as much as possible.
 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).
 I am not exactly sure but it might be that the method
is_t_design
already implements this. The parameters would bev=k^2+k+1
,t=2
andlambda=1
.
Vincent
comment:3 Changed 7 years ago by
 Commit changed from 0824ca649d08566381eb74d525744cf3c4aafeb4 to 980680371fce74966baa1982a0d0ed24c6cd7a03
Branch pushed to git repo; I updated commit sha1. New commits:
9806803  Add example, considered is_t_design method, errors removed

comment:4 followup: ↓ 5 Changed 7 years ago by
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
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 7 years ago by
Replying to ncohen:
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
comment:6 in reply to: ↑ 5 ; followup: ↓ 7 Changed 7 years ago by
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
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 7 years ago by
Replying to ncohen:
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 existingis_group_divisible_design
is_projective_plane
would be better as a function indesigns_pyx.pyx
Vincent
comment:8 in reply to: ↑ 7 Changed 7 years ago by
Could you precise what you think:
 there is no need for
is_projective_plane
is_projective_plane
should call the existingis_group_divisible_design
is_projective_plane
would be better as a function indesigns_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
comment:9 Changed 7 years ago by
 Commit changed from 980680371fce74966baa1982a0d0ed24c6cd7a03 to 6e9489d7eb547aaa5be6a173ab2df1e73f3809cb
Branch pushed to git repo; I updated commit sha1. New commits:
6e9489d  Changed the wrong reference to GroupDivisibleDesign

comment:10 Changed 7 years ago by
 Commit changed from 6e9489d7eb547aaa5be6a173ab2df1e73f3809cb to e8c9895d0506eb3a68419915bf314f72a1863610
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e8c9895  Add is_projective_plane to designs_pyx

comment:11 Changed 7 years ago by
 Status changed from needs_work to needs_review
Hello, Now is_projective_plane is a function in designs_pyx.pyx. I also rebased on the last development version.
comment:12 Changed 7 years ago by
 Milestone changed from sage6.7 to sage6.8
 Status changed from needs_review to positive_review
comment:13 followup: ↓ 15 Changed 7 years ago by
length of liness?
comment:14 Changed 7 years ago by
also, I do not get exactly why you did not use is_pairwise_balanced_design
directly, since it fills the groups for you.
Nathann
comment:15 in reply to: ↑ 13 Changed 7 years ago by
comment:16 Changed 7 years ago by
You know, the old 80chr width stuff.
comment:17 Changed 7 years ago by
 Branch changed from u/q.honore/is_projective_plane to e8c9895d0506eb3a68419915bf314f72a1863610
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Add is_projective_plane to incidence_structure