Opened 8 years ago

Closed 7 years ago

#14883 closed defect (fixed)

Weird multiplication by identity in

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 darij)

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)

trac_14883-remove_times_id-dg.patch (1.6 KB) - added by darij 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by darij

comment:1 Changed 8 years ago by darij

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by chapoton

  • Dependencies changed from 14772 to #14772

patch does not apply, maybe because the dependency is not written correctly, let us see.

comment:3 Changed 8 years ago by darij

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 chapoton

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 darij

Oh, the patchbot. I'm not (yet) smart enough to understand its output...

comment:6 Changed 7 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Looks good to me.

comment:7 Changed 7 years ago by jdemeyer

  • 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 tscrim

  • Milestone changed from sage-pending to sage-5.12

comment:9 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.12.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.