Opened 9 years ago

Closed 8 years ago

#5538 closed defect (fixed)

[with patch; postiive review] Family does copy its input + various improvement.

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-3.4.1
Component: combinatorics Keywords: Family, mutable input
Cc: sage-combinat Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by hivert)

When family got a dictionary it does not copy it's input so that one can modify it. Before the patch we had the following wrong behavior:

sage: d = {1:"a", 3:"b", 4:"c"}
sage: f = Family(d); f
Finite family {1: 'a', 3: 'b', 4: 'c'}
sage: d[2] = 'DD'
sage: f
Finite family {1: 'a', 2: 'DD', 3: 'b', 4: 'c'}

This is now corrected.

The second improvement is that list, and tuple can now be transformed to family indexed by 0..n with the class TrivialFamily?;

The third improvement is that FiniteCombinatorialClass? is now fully compatible with family.

A Fourth improvement is that for lazy family the pickling of the function or the attrcall is done as far as possible. And example of a case where it is not possible to work has been added.

Finally since family has noting to do with combinatorics, I moved this to the more sensible sage.set.

Author: Florent Hivert

Attachments (7)

family_improve-fh-5538-submitted.patch (48.1 KB) - added by hivert 8 years ago.
family_doc_fix-final.patch (914 bytes) - added by hivert 8 years ago.
Doc fix
family.diff (8.9 KB) - added by hivert 8 years ago.
Difference of family.py before and after the patch.
family_interface-cleanup-fh-final.patch (14.7 KB) - added by hivert 8 years ago.
Cleanup of the interface.
family_adapt-fh-final.patch (3.2 KB) - added by hivert 8 years ago.
Adapted root system with the new interface
family_review.patch (2.6 KB) - added by nthiery 8 years ago.
family_pickle_fix-fh.patch (8.4 KB) - added by hivert 8 years ago.
Fixing the Pickle + Minimal doc header.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by hivert

  • Summary changed from Family does not copy it's input. to Family does not copy its input.

Changed 8 years ago by hivert

comment:2 Changed 8 years ago by hivert

  • Summary changed from Family does not copy its input. to [with patch; needs review] Family does copy its input + various improvement.

The uploaded patch improve family in several ways.

  • first of all the input is now systematically copied to avoid modification;
  • now family can handle list and tuple with the class TrivialFamily;
  • I also corrected the pickling of function when possible and attrcall;
  • since it has nothing to do with combinatorics I moved family to sage.set

Cheers,

Florent

Changed 8 years ago by hivert

Doc fix

comment:3 Changed 8 years ago by hivert

  • Description modified (diff)

Since the file is moved, the patch is not very useful. I added a diff from the former sage/combinat/family.py to the new sage/sets/family.py. It is *not* a patch and should not be applied.

Changed 8 years ago by hivert

Difference of family.py before and after the patch.

comment:4 Changed 8 years ago by hivert

On irc, it was decided that we should take chance of this patch to finish the cleanup of the interface of family. I've taken care of this but several issues are still open:

  1. The former implementation of family accepted a parameter "name" which was never used and which was there to provide a functionality similar to SageObject.rename. Therefore it should be removed. Is it ok to keep it and raise a warning so that people in sage-combinat adapt their code according to ?
  1. On the other hand, for LazyFamily I can use the name of the function to generate a proper name for the family. Here are some examples:
    sage: def fun(i): 2*i
    sage: f = LazyFamily([3,4,7], fun); f
    Lazy family (fun(i))_{i in [3, 4, 7]}
    
    sage: f = Family(Permutations(3), attrcall("to_lehmer_code"), lazy=True); f
    Lazy family (i.to_lehmer_code())_{i in Standard permutations of 3}
    
    sage: f = LazyFamily([3,4,7], lambda i: 2*i); f
    Lazy family (<lambda>(i))_{i in [3, 4, 7]}
    

Is this ok ? In particular the last one used to be printed

Lazy family (f(i))_{i in [3, 4, 7]}

I find <lambda> more explicit.

  1. To have a single interface it was also decided to add a keyword parameter lazy which call's LazyFamily. The parameter is False by for now default. I think it depends on the input. In particular if index is a combinatorial class which is not a finite one, then the former implementation turned lazy by default. Should we do that ?

Cheers,

Florent

comment:5 Changed 8 years ago by nthiery

Agreed on 1, 2, 3. I very much like the printing in 2. I looked at the current diff, and it looks good to me.

Just two suggestions in TrivialFamily?:

  • iter could return self.set.iter()
  • self.set is a bit of a misnomer since the order is relevant. self.enumeration?

comment:6 Changed 8 years ago by nthiery

  • Cc sage-combinat added

Changed 8 years ago by hivert

Cleanup of the interface.

Changed 8 years ago by hivert

Adapted root system with the new interface

comment:7 Changed 8 years ago by hivert

The current versions of the patch should be definitive ! Please review. They are four patch that should applied without problem in the following order:

The last two clean the interface up and adapt part of combinat and in particular root-systems to the new interface. The test should runs without deprecation warning.

Cheers,

Florent

Changed 8 years ago by nthiery

comment:8 Changed 8 years ago by nthiery

Positive review.

Florent: please update the summary accordingly, after double checking my (trivial) review patch. It needs to be applied last.

Michael: do you mind setting the gard +3_4_1 on the patch server just after the merge? (and possibly also for the other recently merged in sage-combinat patches). Thanks!

comment:9 Changed 8 years ago by nthiery

I meant: +3_4_1 or +3_4_2 depending on where it goes of course.

comment:10 Changed 8 years ago by hivert

  • Summary changed from [with patch; needs review] Family does copy its input + various improvement. to [with patch; positive review] Family does copy its input + various improvement.

Reviewed the review patch. All light green

Florent

comment:11 Changed 8 years ago by hivert

  • Milestone changed from sage-3.4.2 to sage-3.4.1

Dear Michael,

If it's still possible I'd like to see this one merged in 3.4.1. This is a basic stuff that is useful and should be advertised.

Cheers,

Florent

comment:12 Changed 8 years ago by hivert

  • Status changed from new to assigned

comment:13 Changed 8 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.4.2
  • Summary changed from [with patch; positive review] Family does copy its input + various improvement. to [with patch; needs work] Family does copy its input + various improvement.

This patch breaks a lot of pickles:

    Failed:
    _class__sage_combinat_family_FiniteFamilyWithHiddenKeys__.sobj
    _class__sage_combinat_family_FiniteFamily__.sobj
    _class__sage_combinat_family_LazyFamily__.sobj
    _class__sage_combinat_finite_class_FiniteCombinatorialClass_l__.sobj
    _class__sage_combinat_free_module_CombinatorialFreeModuleElement__.sobj
    _class__sage_combinat_free_module_CombinatorialFreeModule__.sobj
    _class__sage_combinat_root_system_root_space_RootSpace__.sobj
    _class__sage_combinat_root_system_type_A_ambient_space__.sobj
    _class__sage_combinat_root_system_type_C_ambient_space__.sobj
    _class__sage_combinat_root_system_type_E_ambient_space__.sobj
    _class__sage_combinat_root_system_type_F_ambient_space__.sobj
    _class__sage_combinat_root_system_type_G_ambient_space__.sobj
    _class__sage_combinat_root_system_type_reducible_CartanType__.sobj
    _class__sage_combinat_root_system_weight_space_WeightSpace__.sobj
    _class__sage_combinat_root_system_weyl_characters_WeightRing__.sobj
    _class__sage_combinat_root_system_weyl_characters_WeylCharacterRing_class__.sobj
    _class__sage_combinat_root_system_weyl_characters_WeylCharacter__.sobj
    _class__sage_combinat_root_system_weyl_group_WeylGroupElement__.sobj
    _class__sage_combinat_root_system_weyl_group_WeylGroup_gens__.sobj
    Successfully unpickled 464 objects.
    Failed to unpickle 19 objects.

Cheers,

Michael

comment:14 Changed 8 years ago by hivert

  • Milestone changed from sage-3.4.2 to sage-3.4.1
  • Summary changed from [with patch; needs work] Family does copy its input + various improvement. to [with patch; needs review] Family does copy its input + various improvement.

Oups ! The problem is due to a file which has been moved and a class which has been renamed. Everything was ready to correctly pickle, but I mixed things up in my backward compatibility import links. I just updated a simple patch which solve the problem on my machine.

Michael: can you review it ? You are the master of checking pickle !

I'm still hoping to be in time for 3.4.1 ...

Sorry for the mess,

Florent

Changed 8 years ago by hivert

Fixing the Pickle + Minimal doc header.

comment:15 Changed 8 years ago by mabshoff

  • Summary changed from [with patch; needs review] Family does copy its input + various improvement. to [with patch; postiive review] Family does copy its input + various improvement.

Positive review for the pickle fix patch. The five patches together make the test suite pass.

Cheers,

Michael

comment:16 Changed 8 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged

  • trac_5538_part_1_family_improve-fh-5538-submitted.patch
  • trac_5538_part_2_family_doc_fix-final.patch
  • trac_5538_part_3_family_interface-cleanup-fh-final.patch
  • trac_5538_part_4_family_adapt-fh-final.patch
  • trac_5538_part_5_family_pickle_fix-fh.patch

in Sage 3.4.1.rc3.

Cheers,

Michael

Note: See TracTickets for help on using tickets.