Opened 8 years ago
Closed 7 years ago
#14883 closed defect (fixed)
Weird multiplication by identity in set_partition_ordered.py
Reported by: | darij | Owned by: | sage-combinat |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.12 |
Component: | combinatorics | Keywords: | combinat, ordered set partitions, permutations |
Cc: | Merged in: | sage-5.12.beta4 | |
Authors: | Darij Grinberg | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14772, #8386, #14519, #14808, #14143, #14015, #14516 | Stopgaps: |
Description (last modified by )
See the attached patch which, I hope, speaks for itself. I don't see any reason why that permutation had to be multiplied by an identity permutation. I thought it might have been meant to coerce the permutation into a larger symmetric group or to avoid some corner-case bugs, but this doesn't seem to be an issue (cf. the doctests I added). Also, notice that OrderedSetPartitions
class does already check for the set and the composition to be of the same size, whereas the un-exported OrderedSetPartitions_scomp
class throws various errors if they aren't (both before and after my patch). The patch has a minor but nontrivial effect on speed:
sage: timeit("list(OrderedSetPartitions([1,2,3,4,5],[3,2]))")
now takes 6.75 rather than 7.67 ms.
Attachments (1)
Change History (10)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Dependencies changed from 14772 to #14772
comment:3 Changed 8 years ago by
Oops, the dependency definitely needed a hash. But how does that affect the failure of the patch? Do you have a bot which pulls dependencies automatically?
comment:4 Changed 8 years ago by
yes, the patchbot is smart enough to do that, indeed. Click on the red circle and you will see !
comment:5 Changed 8 years ago by
Oh, the patchbot. I'm not (yet) smart enough to understand its output...
comment:6 Changed 7 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Looks good to me.
comment:7 Changed 7 years ago by
- Dependencies changed from #14772 to #14772, #8386, #14519, #14808, #14143, #14015, #14516
- Milestone changed from sage-5.12 to sage-pending
comment:8 Changed 7 years ago by
- Milestone changed from sage-pending to sage-5.12
comment:9 Changed 7 years ago by
- Merged in set to sage-5.12.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
patch does not apply, maybe because the dependency is not written correctly, let us see.