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: 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: e8c9895 (Commits, GitHub, GitLab) Commit: e8c9895d0506eb3a68419915bf314f72a1863610
Dependencies: Stopgaps:

Status badges

Description

Add new method ìs_projective_plane to the class IncidenceStructure

Change History (17)

comment:1 Changed 7 years ago by q.honore

  • Branch set to u/q.honore/is_projective_plane
  • Commit set to 0824ca649d08566381eb74d525744cf3c4aafeb4
  • Status changed from new to needs_review

New commits:

0824ca6Add is_projective_plane to incidence_structure

comment:2 Changed 7 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

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.
  1. 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 k2 + k + 1, in other words
    The number of points must be k^2 + k + 1 = 6, got 7
    
  1. 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.
  1. 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.
  1. 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).
  1. 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

comment:3 Changed 7 years ago by git

  • Commit changed from 0824ca649d08566381eb74d525744cf3c4aafeb4 to 980680371fce74966baa1982a0d0ed24c6cd7a03

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

9806803Add example, considered is_t_design method, errors removed

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

  • 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 ; follow-up: Changed 7 years ago by vdelecroix

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 ; follow-up: Changed 7 years ago by 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.

Would be cool to make the correction in this ticket.

+1

Nathann

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by vdelecroix

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 existing is_group_divisible_design
  • is_projective_plane would be better as a function in designs_pyx.pyx

Vincent

comment:8 in reply to: ↑ 7 Changed 7 years ago by ncohen

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

comment:9 Changed 7 years ago by git

  • Commit changed from 980680371fce74966baa1982a0d0ed24c6cd7a03 to 6e9489d7eb547aaa5be6a173ab2df1e73f3809cb

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

6e9489dChanged the wrong reference to GroupDivisibleDesign

comment:10 Changed 7 years ago by git

  • Commit changed from 6e9489d7eb547aaa5be6a173ab2df1e73f3809cb to e8c9895d0506eb3a68419915bf314f72a1863610

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

e8c9895Add is_projective_plane to designs_pyx

comment:11 Changed 7 years ago by q.honore

  • 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 vdelecroix

  • Milestone changed from sage-6.7 to sage-6.8
  • Status changed from needs_review to positive_review

comment:13 follow-up: Changed 7 years ago by ncohen

length of liness?

comment:14 Changed 7 years ago by ncohen

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 vdelecroix

Replying to ncohen:

length of liness?

??

comment:16 Changed 7 years ago by ncohen

You know, the old 80chr width stuff.

comment:17 Changed 7 years ago by vbraun

  • Branch changed from u/q.honore/is_projective_plane to e8c9895d0506eb3a68419915bf314f72a1863610
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.