Opened 5 years ago

Closed 5 years ago

#16032 closed defect (fixed)

Bug in IncidenceStructure.dual_design

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.2
Component: combinatorics Keywords:
Cc: dimpase Merged in:
Authors: Nathann Cohen Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: f6da4b7 (Commits) 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 5 years ago by ncohen

  • Branch set to u/ncohen/16032
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to fed439e930439205b79831251e528bfc6065ffec

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

fed439etrac #16032: Bug in IncidenceStructure.dual_design

comment:3 Changed 5 years ago by dimpase

What is the bug in question?

comment:4 follow-up: Changed 5 years ago by dimpase

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

Last edited 5 years ago by dimpase (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 5 years ago by ncohen

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

sage: 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 about algorithm= 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.py 

fails.

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

  • Commit changed from fed439e930439205b79831251e528bfc6065ffec to f82b15d762f97b65ab38e6d1f79af210c9b8a7e3

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

f82b15dtrac #16032: reviewer's remarks

comment:7 Changed 5 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

OK!

comment:8 Changed 5 years ago by ncohen

You don't mind my removing this "if", do you ? :-PPPPP

Nathann

comment:9 Changed 5 years ago by git

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

7dcff95trac 16032: This can be simplified... :-P

comment:10 Changed 5 years ago by dimpase

shouldn't these matrices be created sparse?

comment:11 follow-up: Changed 5 years ago by ncohen

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

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 5 years ago by ncohen

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 re-do the work when they turn their back.

Nathann

Last edited 5 years ago by ncohen (previous) (diff)

comment:14 Changed 5 years ago by git

  • Commit changed from 7dcff95e93b99377e996ab4ed37b6eaf78944e37 to f6da4b77b3c0067eaab3b411720998e9c9cf07c4

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

f6da4b7trac #16032: These matrices should be sparse

comment:15 Changed 5 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:16 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/16032 to f6da4b77b3c0067eaab3b411720998e9c9cf07c4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.