Opened 8 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 )
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)
Change History (23)
comment:1 Changed 8 years ago by
- Summary changed from Family does not copy it's input. to Family does not copy its input.
Changed 8 years ago by
comment:2 Changed 8 years ago by
- Summary changed from Family does not copy its input. to [with patch; needs review] Family does copy its input + various improvement.
comment:3 Changed 8 years ago by
- 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.
comment:4 Changed 8 years ago by
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:
- 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 ?
- 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.
- 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 ifindex
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
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
- Cc sage-combinat added
comment:7 Changed 8 years ago by
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
comment:8 Changed 8 years ago by
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
I meant: +3_4_1 or +3_4_2 depending on where it goes of course.
comment:10 Changed 8 years ago by
- 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
- 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
- Status changed from new to assigned
comment:13 Changed 8 years ago by
- 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
- 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
comment:15 Changed 8 years ago by
- 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
- 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
The uploaded patch improve family in several ways.
TrivialFamily
;Cheers,
Florent