Bug in IncidenceStructure.dual_design #16032

nathanncohen · 2014-03-30T20:54:44Z

As the title says.

With some other improvements.

The original code seems to have been written by a mathematician.

Nathann

CC: @dimpase

Component: combinatorics

Author: Nathann Cohen

Branch/Commit: f6da4b7

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/16032

The text was updated successfully, but these errors were encountered:

nathanncohen · 2014-03-30T20:55:31Z

Branch: u/ncohen/16032

sagetrac-git · 2014-03-30T20:57:13Z

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

`fed439e`	`trac #16032: Bug in IncidenceStructure.dual_design`

sagetrac-git · 2014-03-30T20:57:13Z

Commit: fed439e

dimpase · 2014-03-30T22:05:34Z

comment:3

What is the bug in question?

dimpase · 2014-03-30T22:47:14Z

comment:4

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.

nathanncohen · 2014-03-31T07:58:54Z

comment:5

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

sagetrac-git · 2014-03-31T07:59:25Z

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

`f82b15d`	`trac #16032: reviewer's remarks`

sagetrac-git · 2014-03-31T07:59:25Z

Changed commit from fed439e to f82b15d

dimpase · 2014-03-31T08:53:17Z

comment:7

OK!

dimpase · 2014-03-31T08:53:17Z

Reviewer: Dima Pasechnik

nathanncohen · 2014-03-31T12:33:44Z

comment:8

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

Nathann

sagetrac-git · 2014-03-31T12:33:54Z

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`

sagetrac-git · 2014-03-31T12:33:54Z

Changed commit from f82b15d to 7dcff95

dimpase · 2014-03-31T12:39:06Z

comment:10

shouldn't these matrices be created sparse?

nathanncohen · 2014-03-31T12:40:23Z

comment:11

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

dimpase · 2014-03-31T12:43:37Z

comment:12

Replying to @nathanncohen:

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

nathanncohen · 2014-03-31T12:50:00Z

comment:13

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

sagetrac-git · 2014-03-31T12:50:17Z

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

`f6da4b7`	`trac #16032: These matrices should be sparse`

sagetrac-git · 2014-03-31T12:50:17Z

Changed commit from 7dcff95 to f6da4b7

vbraun · 2014-03-31T21:12:31Z

Changed branch from u/ncohen/16032 to f6da4b7

* develop: (155 commits) Updated Sage version to 6.2.beta6 32-bit fix typo fixing fix int/long doctest trac sagemath#16032: These matrices should be sparse trac 16032: This can be simplified... :-P Correct typo in sage/symbolic/ring.pyx. Put comment in the code and add more examples. trac sagemath#16032: reviewer's remarks FiniteStateMachine.__add__ is the same as __or__ trac sagemath#16032: Bug in IncidenceStructure.dual_design final edits more changes -- please check if I got your algorithm right, Travis Review changes from Ben. further optimizations Fixed KR doctest and documentation of q_dimension. Fixes for q-dims for general highest weight crystals. more optimizations Partial work for general HW crystals. Review changes and (minor) optimizations. ...

nathanncohen mannequin added this to the sage-6.2 milestone Mar 30, 2014

nathanncohen mannequin added c: combinatorics labels Mar 30, 2014

nathanncohen mannequin added the s: needs review label Mar 30, 2014

dimpase added s: needs work and removed s: needs review labels Mar 30, 2014

nathanncohen mannequin added s: needs review and removed s: needs work labels Mar 31, 2014

dimpase added s: positive review and removed s: needs review labels Mar 31, 2014

sagetrac-git mannequin added s: needs review and removed s: positive review labels Mar 31, 2014

dimpase added s: positive review and removed s: needs review labels Mar 31, 2014

vbraun removed the s: positive review label Mar 31, 2014

vbraun closed this as completed in d4d9b99 Mar 31, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug in IncidenceStructure.dual_design #16032

Bug in IncidenceStructure.dual_design #16032

nathanncohen mannequin commented Mar 30, 2014

nathanncohen mannequin commented Mar 30, 2014

sagetrac-git mannequin commented Mar 30, 2014

sagetrac-git mannequin commented Mar 30, 2014

dimpase commented Mar 30, 2014

dimpase commented Mar 30, 2014

nathanncohen mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

dimpase commented Mar 31, 2014

dimpase commented Mar 31, 2014

nathanncohen mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

dimpase commented Mar 31, 2014

nathanncohen mannequin commented Mar 31, 2014

dimpase commented Mar 31, 2014

nathanncohen mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

vbraun commented Mar 31, 2014

Bug in IncidenceStructure.dual_design #16032

Bug in IncidenceStructure.dual_design #16032

Comments

nathanncohen mannequin commented Mar 30, 2014

nathanncohen mannequin commented Mar 30, 2014

sagetrac-git mannequin commented Mar 30, 2014

sagetrac-git mannequin commented Mar 30, 2014

dimpase commented Mar 30, 2014

dimpase commented Mar 30, 2014

nathanncohen mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

dimpase commented Mar 31, 2014

dimpase commented Mar 31, 2014

nathanncohen mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

dimpase commented Mar 31, 2014

nathanncohen mannequin commented Mar 31, 2014

dimpase commented Mar 31, 2014

nathanncohen mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

sagetrac-git mannequin commented Mar 31, 2014

vbraun commented Mar 31, 2014