Opened 7 years ago
Closed 7 years ago
#16032 closed defect (fixed)
Bug in IncidenceStructure.dual_design
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  combinatorics  Keywords:  
Cc:  dimpase  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  f6da4b7 (Commits, GitHub, GitLab)  Commit:  f6da4b77b3c0067eaab3b411720998e9c9cf07c4 
Dependencies:  Stopgaps: 
Description
As the title says.
With some other improvements.
The original code seems to have been written by a mathematician.
Nathann
Change History (16)
comment:1 Changed 7 years ago by
 Branch set to u/ncohen/16032
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Commit set to fed439e930439205b79831251e528bfc6065ffec
comment:3 Changed 7 years ago by
What is the bug in question?
comment:4 followup: ↓ 5 Changed 7 years ago by
 Status changed from needs_review to needs_work
there is a bug in the optional test: the line 422 in incidence_structures.py
should be
sage: PP = designs.ProjectiveGeometryDesign(2,1,GF(4,'conway'), algorithm="gap") # optional  gap_design
actually, it's weird that ProjectivePlaneDesign
does not known about algorithm=
at all.
This is a bug to be fixed, IMHO, perhaps on this ticket...
the patch is:
 a/src/sage/combinat/designs/incidence_structures.py +++ b/src/sage/combinat/designs/incidence_structures.py @@ 419,7 +419,7 @@ class IncidenceStructure(object): sage: PP = designs.ProjectivePlaneDesign(4) sage: PP.dual_design().is_block_design() (True, [2, 21, 5, 1])  sage: PP = designs.ProjectivePlaneDesign(4, algorithm="gap") # optional  gap_design + sage: PP = designs.ProjectiveGeometryDesign(2,1,GF(4,'conway'), algorithm="gap") # optional  gap_design sage: PP.dual_design().is_block_design() # optional  gap_design (True, [2, 21, 5, 1])
without it, the test
sage t long optional=sage,gap_design src/sage/combinat/designs/incidence_structures.py
fails.
comment:5 in reply to: ↑ 4 Changed 7 years ago by
 Status changed from needs_work to needs_review
Yoooooooo !
there is a bug in the optional test: the line 422 in
incidence_structures.py
should besage: PP = designs.ProjectiveGeometryDesign(2,1,GF(4,'conway'), algorithm="gap") # optional  gap_design
Nononono, I meant this "algorithm=gap" to be an argument of the function I tests, i.e. dual_design
.
And I rename this gap_design
(which is not even the name of a spkg) to gap_packages
.
actually, it's weird that
ProjectivePlaneDesign
does not known aboutalgorithm=
at all. This is a bug to be fixed, IMHO, perhaps on this ticket...
The standard of the bugs I fix is MUCH higher than the bugs that are not fixed in combinat, really. If THIS is a bug their code should not even be allowed to run :P
without it, the test
sage t long optional=sage,gap_design src/sage/combinat/designs/incidence_structures.pyfails.
Well, as I said above the "algorithm=gap" was intended for the dual_block
method. I don't see why we would call gap to compute a dual, but anyway :P
Aaaaaaaaaaaaaand I added this algorithm
argument in ProjectivePlane
too.
Nathann
comment:6 Changed 7 years ago by
 Commit changed from fed439e930439205b79831251e528bfc6065ffec to f82b15d762f97b65ab38e6d1f79af210c9b8a7e3
Branch pushed to git repo; I updated commit sha1. New commits:
f82b15d  trac #16032: reviewer's remarks

comment:7 Changed 7 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
OK!
comment:8 Changed 7 years ago by
You don't mind my removing this "if", do you ? :PPPPP
Nathann
comment:9 Changed 7 years ago by
 Commit changed from f82b15d762f97b65ab38e6d1f79af210c9b8a7e3 to 7dcff95e93b99377e996ab4ed37b6eaf78944e37
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
7dcff95  trac 16032: This can be simplified... :P

comment:10 Changed 7 years ago by
shouldn't these matrices be created sparse?
comment:11 followup: ↓ 12 Changed 7 years ago by
Ahahaha. Yes of course. Do I change that ?
This code is awful, I am already happy to see those two "for" reverted in order to avoid a useless test. Mathematicians should not write code, all they care about is whether the code can be read like a mathematical proof, and efficiency can go to hell.
Nathann
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 7 years ago by
Replying to ncohen:
Ahahaha. Yes of course. Do I change that ?
yes please...
This code is awful, I am already happy to see those two "for" reverted in order to avoid a useless test. Mathematicians should not write code, all they care about is whether the code can be read like a mathematical proof, and efficiency can go to hell.
they can be taught to write good code, IMHO :)
comment:13 in reply to: ↑ 12 Changed 7 years ago by
Yooooooo !
yes please...
Done !
they can be taught to write good code, IMHO :)
Probably. But most of your efforts will be wasted, and in the meantime you have to redo the work when they turn their back.
Have you looked at #15664 ? :P
Nathann
comment:14 Changed 7 years ago by
 Commit changed from 7dcff95e93b99377e996ab4ed37b6eaf78944e37 to f6da4b77b3c0067eaab3b411720998e9c9cf07c4
Branch pushed to git repo; I updated commit sha1. New commits:
f6da4b7  trac #16032: These matrices should be sparse

comment:15 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:16 Changed 7 years ago by
 Branch changed from u/ncohen/16032 to f6da4b77b3c0067eaab3b411720998e9c9cf07c4
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #16032: Bug in IncidenceStructure.dual_design