Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird multiplication by identity in set_partition_ordered.py #14883

Closed
darijgr opened this issue Jul 12, 2013 · 11 comments
Closed

Weird multiplication by identity in set_partition_ordered.py #14883

darijgr opened this issue Jul 12, 2013 · 11 comments

Comments

@darijgr
Copy link
Contributor

darijgr commented Jul 12, 2013

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.

Depends on #14772
Depends on #8386
Depends on #14519
Depends on #14808
Depends on #14143
Depends on #14015
Depends on #14516

Component: combinatorics

Keywords: combinat, ordered set partitions, permutations

Author: Darij Grinberg

Reviewer: Travis Scrimshaw

Merged: sage-5.12.beta4

Issue created by migration from https://trac.sagemath.org/ticket/14883

@darijgr

This comment has been minimized.

@darijgr
Copy link
Contributor Author

darijgr commented Jul 12, 2013

comment:1

Attachment: trac_14883-remove_times_id-dg.patch.gz

@fchapoton
Copy link
Contributor

comment:2

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

@fchapoton
Copy link
Contributor

Changed dependencies from 14772 to #14772

@darijgr
Copy link
Contributor Author

darijgr commented Jul 12, 2013

comment:3

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?

@fchapoton
Copy link
Contributor

comment:4

yes, the patchbot is smart enough to do that, indeed. Click on the red circle and you will see !

@darijgr
Copy link
Contributor Author

darijgr commented Jul 12, 2013

comment:5

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

@tscrim
Copy link
Collaborator

tscrim commented Jul 20, 2013

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 20, 2013

comment:6

Looks good to me.

@jdemeyer
Copy link

jdemeyer commented Aug 2, 2013

Changed dependencies from #14772 to #14772, #8386, #14519, #14808, #14143, #14015, #14516

@jdemeyer jdemeyer removed this from the sage-5.12 milestone Aug 2, 2013
@tscrim tscrim added this to the sage-5.12 milestone Aug 11, 2013
@tscrim tscrim removed the pending label Aug 11, 2013
@jdemeyer
Copy link

Merged: sage-5.12.beta4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants