Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6903 closed defect (fixed)

[with patch, positive review] Function Word currently prevent the inheritance of Words_over_OrderedAlphabet

Reported by: slabbe Owned by: slabbe
Priority: major Milestone: sage-4.1.2
Component: combinatorics Keywords:
Cc: saliola Merged in: Sage 4.1.2.alpha2
Authors: Sébastien Labbé, Franco Saliola Reviewers: Franco Saliola
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Let

sage: W = Words('ab')

In the state of sage-4.1.1, the function W.__call__ uses the function Word....but it should be the inverse. In fact, the code of the function Word contains code like :

[...]

    # Construct the word
    if datatype == 'list':
        w = FiniteWord_list(parent=parent,data=data)
    elif datatype == 'str':
        w = FiniteWord_str(parent=parent,data=data)
    elif datatype == 'tuple':
        w = FiniteWord_tuple(parent=parent,data=data)
    elif datatype == 'callable':
        if caching:
            if length is None or length is Infinity:
                cls = InfiniteWord_callable_with_caching
            else:
                cls = FiniteWord_callable_with_caching
        else:
            if length is None or length is Infinity:
                cls = InfiniteWord_callable
            else:
                cls = FiniteWord_callable
        w = cls(parent=parent,callable=data,length=length)

[...]

The problems come when someone wants to inherits the class Words_over_OrderedAlphabet to create, let say, a combinatorial classes of all paths (see #5038). Below, we would like the __call__ function of WordPaths_all to creates words paths instances and not the usual words instances. I don't want to rewrite the __call__ function for WordPaths_all since it could be the exact same thing as the one of Words_over_OrderedAlphabet.

class WordPaths_all(Words_over_OrderedAlphabet):
    r"""
    The combinatorial class of all paths, i.e of all words over
    an alphabet where each letter is mapped to a step (a vector).
    """
    def __init__(self, alphabet, steps):
        r"""
        INPUT:

        - ``alphabet`` - an ordered alphabet 

        - ``steps`` - an iterable (of same length as alphabet) of ordered vectors

        EXAMPLES::

[...]

One solution is that the current code of Word goes to Words.__call__ and that the function Word simply creates the parent from the input alphabet and delegates the creation to the parent.

Attachments (4)

trac_6903_word_improve_constructor-sl.patch (17.8 KB) - added by slabbe 5 years ago.
trac_6903_reviewer_patch_1.patch (7.2 KB) - added by saliola 5 years ago.
Apply on top of trac_6903_word_improve_constructor-sl.patch
trac_6903_reviewer_patch_2.patch (2.2 KB) - added by saliola 5 years ago.
Apply on top of trac_6903_reviewer_patch_2.patch
trac_6903_review_3-sl.patch (1.2 KB) - added by slabbe 5 years ago.
Applies over the precedent 3 patches.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by slabbe

  • Summary changed from Function Word currently prevent the inheritance of Words_over_OrderedAlphabet to [with patch, needs work] Function Word currently prevent the inheritance of Words_over_OrderedAlphabet

I just posted a patch which corresponds to the work around I recently was using. Something better could be done.

Dear Franco, I would like to discuss with you about it, and I could then implement something better.

Changed 5 years ago by slabbe

comment:2 Changed 5 years ago by slabbe

  • Owner changed from mhansen to slabbe
  • Status changed from new to assigned

I just posted a patch which correspond to something better. All test pass in sage/combinat/words. I also tested the pickle jar using (from http://www.sagemath.org/doc/reference/sage/structure/sage_object.html) :

sage: std = os.environ['SAGE_DATA'] + '/extcode/pickle_jar/pickle_jar.tar.bz2'
sage: sage.structure.sage_object.unpickle_all(std)

I keep the 'needs work' label, because there is still something I want to check. In fact, the code that changes the class of a word from Word_class to FiniteWord_class when it reaches the end of the word might be updated as well here, but I can't figure out where is that code!!! Franco should remember that.

comment:3 Changed 5 years ago by saliola

I am including two patches here.

In trac_6903_reviewer_patch_1.patch, I made the following changes:

  1. I fixed the code that changes the class of a word FiniteWord_class when it reaches the end of the iterator.
  1. I changed _classetype to _element_classes since the method defines the classes of the elements of the parent.
  1. I also cleaned up the code for _element_classes.
  1. Both Words_all.__call__ and Words_all._construct_word run self._check, so I am deleting the occurrence in Words_all._construct_word.

In trac_6903_reviewer_patch_2.patch, I changed the method _element_classes into an attribute (a lazy_attribute, actually). So it *is* the dictionary instead of returning the dictionary. I think it is a bad idea to re-create that dictionary every time a word is created.

Changed 5 years ago by saliola

Apply on top of trac_6903_word_improve_constructor-sl.patch

Changed 5 years ago by saliola

Apply on top of trac_6903_reviewer_patch_2.patch

comment:4 Changed 5 years ago by saliola

  • Summary changed from [with patch, needs work] Function Word currently prevent the inheritance of Words_over_OrderedAlphabet to [with patch, needs review] Function Word currently prevent the inheritance of Words_over_OrderedAlphabet

Modulo the changes in the reviewer patches, I give a positive review to Sébastien's patch.

Sébastien, can you review my patches?

Changed 5 years ago by slabbe

Applies over the precedent 3 patches.

comment:5 Changed 5 years ago by slabbe

  • Authors set to Sébastien Labbé, Franco Saliola
  • Reviewers set to Franco Saliola

I just added a patch that removes two lines now useless after Franco's changes.

All test passed in sage/combinat/words directory. Documentation builds w/o warnings. I am giving a positive review to Franco's changes. I let Franco change the status of the ticket since I posted the last patch.

comment:6 Changed 5 years ago by saliola

  • Summary changed from [with patch, needs review] Function Word currently prevent the inheritance of Words_over_OrderedAlphabet to [with patch, positive review] Function Word currently prevent the inheritance of Words_over_OrderedAlphabet

Thanks for catching those two lines Sébastien. Positive review.

comment:7 Changed 5 years ago by mvngu

  • Merged in set to Sage 4.1.2.alpha2
  • Resolution set to fixed
  • Status changed from assigned to closed

Merged patches in this order:

  1. trac_6903_word_improve_constructor-sl.patch
  2. trac_6903_reviewer_patch_1.patch
  3. trac_6903_reviewer_patch_2.patch
  4. trac_6903_review_3-sl.patch

comment:8 Changed 5 years ago by mvngu

  • Milestone changed from sage-combinat to sage-4.1.2
Note: See TracTickets for help on using tickets.