LinearCode.cardinality category breakage

[ppurka]

sage: C = codes.HammingCode(3, GF(2))
sage: C.cardinality()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-2e23aafacb95> in <module>()
----> 1 C.cardinality()

/home/release/Sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__getattr__ (build/cythonized/sage/structure/parent.c:7211)()

AttributeError: 'LinearCode' object has no attribute 'cardinality'
sage: 'cardinality' in dir(C)
True
sage: hasattr(C, 'cardinality')
False

[ncohen]

(the same happens with c.cartesian_product and c.sum, and probably others too... :-/)

[ppurka]

Replying to ncohen:

(the same happens with c.cartesian_product and c.sum, and probably others too... :-/)

Yikes! Where are all these methods coming from? They are not in any of the classes that LinearCode inherits from.

---

New commits:

​af3e48e	initial commit to enable thickness of edges in graph plots
​be6042d	introduce thickness parameter in scatter_plot
​fe12334	add cardinality() method to LinearCode

[ncohen]

Yikes! Where are all these methods coming from? They are not in any of the classes that LinearCode inherits from.

Don't ask me, I always blame it on categories whatever happens.

Nathann

[git]

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

​4ccae66

add cardinality() method to LinearCode

[ppurka]

Sorry, I need to sleep. I based previous commit on the wrong branch. Did a forced update now.

[ncohen]

Oh, I see. WHen you will be awake: we had to write a LOT of code in the combinatorial design folders recently, and had queues of 5/6 ticket in needs_review depending on each other: we would have died if we had not added to every commit the ticket number at the beginning of the commit message. Otherwise you have no way to guess which commits belong to a given ticket.

Good night ! ;-)

Nathann

[ncohen]

Should this be in needs_review ?

Nathann

[ppurka]

Yes, of course. Thanks. I think cardinality should definitely be implemented, but am not so clear about cartesian product and sum, and possibly a bunch of others.

[ncohen]

Yes, of course. Thanks. I think cardinality should definitely be implemented, but am not so clear about cartesian product and sum, and possibly a bunch of others.

Well, that's a different problem: why the hell are all those function half-inherited ? T_T

Nathann

[vbraun]

Nicolas, can you explain to me why this is not working. And, more importantly. why do we get totally useless diagnostics for why it is not working?

[tscrim]

I believe it's because it's not a new style parent and it doesn't properly initialize its category:

class LinearCode(module.Module_old):
    def __init__(self, gen_mat, d=None):
        ParentWithGens.__init__(self, base_ring)

and in Module_old:

def category(self):
    """
    Return the category to which this module belongs.
    """
    # Defining a category method is deprecated for parents.
    # Instead, the category should be specified in the constructor.
    # See: http://sagetrac.org/sage_trac/wiki/CategoriesRoadMap
    if self._is_category_initialized():
        return Parent.category(self)
    from sage.categories.modules import Modules
    return Modules(self.base_ring())

[vbraun]

That doesn't explain why it half-works, nor is it an excuse for the total failure to provide a useful error message.

[tscrim]

Because it doesn't initialize its category, we have:

sage: C = codes.HammingCode(3, GF(2))
sage: C.__class__.__mro__
(sage.coding.linear_code.LinearCode,
 sage.modules.module.Module_old,
 sage.structure.parent_gens.ParentWithAdditiveAbelianGens,
 sage.structure.parent_gens.ParentWithGens,
 sage.structure.parent_base.ParentWithBase,
 sage.structure.parent_old.Parent,
 sage.structure.parent.Parent,
 sage.structure.category_object.CategoryObject,
 sage.structure.sage_object.SageObject,
 object)

I believe the __getattr__ in parent.pyx is why it gets listed by tab completion, but because the method doesn't actually exist in the MRO, it can't call it. Simon or Nicolas could probably give a better explanation. IMO it's not worth it to try and contrive a "proper" error message, but instead put a note somewhere that this is a sign that the category framework is being abused.

So here's the branch which changes LinearCode to a new-style parent and all (non-long) tests pass in the coding folding. However because I don't know about codes, I can't implement an _an_element_ for the test suite, nor do I know what the proper/best category is (i.e. are they finite dimensional modules? I'm guessing they are but IDK for sure). However this does fix the issue on this ticket.

---

New commits:

​b48b627

Changed LinearCode into a new-style parent.

[vbraun]

Replying to tscrim:

IMO it's not worth it to try and contrive a "proper" error message

I suggest you read the history of this ticket again. Getting proper error messages is crucial for any programming framework.

[ppurka]

@tscrim: there is still this problem with element_class. I have been able to get some other stuff working with your patch.

sage: C = codes.HammingCode(3, GF(2))                                           
sage: C.cardinality()                                                           
16
sage: C.an_element()
(1, 0, 0, 0, 0, 1, 1)
sage: C2 = C.cartesian_product(C)
sage: C2.an_element()
((1, 0, 0, 0, 0, 1, 1), (1, 0, 0, 0, 0, 1, 1))
sage: C.categories()
[Category of vector spaces over Finite Field of size 2,
 Category of modules over Finite Field of size 2,
 Category of bimodules over Finite Field of size 2 on the left and Finite Field of size 2 on the right,
 Category of right modules over Finite Field of size 2,
 Category of left modules over Finite Field of size 2,
 Category of commutative additive groups,
 Category of additive groups,
 Category of additive inverse additive unital additive magmas,
 Category of commutative additive monoids,
 Category of additive monoids,
 Category of additive unital additive magmas,
 Category of commutative additive semigroups,
 Category of additive commutative additive magmas,
 Category of additive semigroups,
 Category of additive magmas,
 Category of sets,
 Category of sets with partial maps,
 Category of objects]
sage: C.element_class()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-5-66b2e3babdf7> in <module>()
----> 1 C.element_class()

/home/punarbasu/Installations/sage-git/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__getattr__ (build/cythonized/sage/structure/parent.c:7290)()

/home/punarbasu/Installations/sage-git/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (build/cythonized/sage/structure/misc.c:1687)()

AttributeError: 'LinearCode_with_category' object has no attribute 'element_class'

Edit: Also implemented C.zero() so that C.sum() works.

sage: C.zero_element()
(0, 0, 0, 0, 0, 0, 0)
sage: C.sum(()) # indirect doctest
(0, 0, 0, 0, 0, 0, 0)

So, we just need to figure out where this element_class is coming from and fix that.

[git]

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

​740bbd3	implement _an_element_ for LinearCode
​d1fca72	Implement LinearCode.cardinality()
​eef3aaa	implement LinearCode.zero()

[git]

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

​6013027

Fixed LinearCode a facade parent and so that it passes the test suite.

[tscrim]

It comes from the category framework, although I believe it is there more for compatibility with old-style parents.

I've made LinearCode into a facade parent (because I didn't want to deal with changing it over to have a proper element here...yes I'm being somewhat lazy), put it also in the category of FiniteEnumeratedSets (as such, we get cardinality for free), and made it so that it passes the TestSuite.

[vbraun]

The real WTF here is not that LinearCode is misusing the category framework, but the utterly wrong and misleading error message that was produced. In Python, AttributeError means you need to implement that attribute but haven't. Nathan and Punarbasu are experienced developers, and if you manage to confuse them then I can guarantee your that there are 10 potential Sage developers that gave up before they posted anything on trac. If only a handful of people that have talked personally to Nicolas can work with the framework then this is going to kill Sage sooner or later. We need to be able to attract new developers, and hurdles like the one in this ticket will prevent that for sure.

[ppurka]

@tscrim: I think what vbraun brought up needs to be fixed in the category framework somewhere.

Other than that, we can not depend on cardinality of the FiniteEnumeratedSets, even if it comes "for free". It comes at a huge cost because it generates all the elements of the linear code - it makes working with large linear codes difficult. We have a vector space structure here, so we can be much more efficient (which was my original patch in this ticket).

sage: C = codes.ReedSolomonCode(10, 7, GF(11))
sage: %time C.cardinality()                                                                                             
CPU times: user 8.14 s, sys: 60 ms, total: 8.2 s
Wall time: 8.1 s
19487171
sage: %time len(C)
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 62.9 µs
19487171

[git]

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

​8df0577	Merge branch 'public/categories/new_parent_linear_codes-16644' of trac.sagemath.org:sage into public/categories/new_parent_lienar_codes-16644
​235dd8e	Added back in cardinality and made __len__ an alias.
​709878e	Import cleanup of linear_code.py.

[tscrim]

@vbraun I would argue that AttributeError is the correct error: the attribute should be there according to the category, but it is not because it is an old-style parent. I'd rather spend my time fixing the problem and educating people about categories rather than trying to come up with a different error message. This solves the issue in the ticket description and we shouldn't hold up this ticket -- the error message should be on a separate ticket.

@ncohen You actually were right in comment:4. I wonder if you're starting to understand categories. :P

@ppurka I didn't look at/see the __len__ method. I've changed that into cardinality and made __len__ into an alias (so it will appear in the documentation).

[vbraun]

Giving correct diagnostics is a way of educating people about categories, and arguably the best way since you can't be looking over everybody's shoulder all the time. I just caught this ticket by chance, and you would have never noticed.

[ppurka]

@vbraun I think we should open a different ticket for fixing the categories. In this ticket the LinearCode class is changed to use the new category framework. I will have to check the latest changes, but they seem quite ok at first glance.

I will open a different ticket for addressing the error messages given by the categories. Does this sound reasonable to you?

[vbraun]

Fine with me...

[ncohen]

@ncohen You actually were right in comment:4. I wonder if you're starting to understand categories. :P

We are old ennemies now.

Nathann

[ppurka]

This looks good to me. It passes all sage doctests and fails in 3 places which are due to gap (see #16660).

[tscrim]

I get the errors of #16660 with develop, so it's not a dependency IMO.

[vbraun]

Followup at #16707