Skip to content

Include derangements by itself#985

Merged
bbayles merged 23 commits intomasterfrom
derangements-alone
May 6, 2025
Merged

Include derangements by itself#985
bbayles merged 23 commits intomasterfrom
derangements-alone

Conversation

@bbayles
Copy link
Copy Markdown
Collaborator

@bbayles bbayles commented May 1, 2025

This PR grabs the commits from #946 and then drops the functions other than derangements. I'm still where I was when I made this comment, and I think that function would be a nice addition to the library.

@debruijn, are you OK with this? The other function implementations are good, but I think this is the core.

@pochmann3
Copy link
Copy Markdown
Contributor

pochmann3 commented May 2, 2025

Do I see correctly that this is the current version?

def derangements(iterable, r=None):
    """
    ...
    Elements are treated as unique based on their position, not on their value.
    ...
    """
    pool = tuple(iterable)
    xs = tuple(zip(pool))
    for ys in permutations(xs, r=r):
        if any(map(operator.eq, xs, ys)):
            continue
        yield tuple(y[0] for y in ys)

That seems wrong and the combination of zip and eq doesn't make sense to me. Using eq doesn't lead to "Elements are treated as unique based on their position, not on their value." And it makes the zip rather pointless.

The output of

for d in derangements([0, False, 1, True]):
    print(d)

is:

(1, True, 0, False)
(1, True, False, 0)
(True, 1, 0, False)
(True, 1, False, 0)

With operator.is_ (as in my original) it becomes:

(False, 0, True, 1)
(False, 1, True, 0)
(False, True, 0, 1)
(1, 0, True, False)
(1, True, 0, False)
(1, True, False, 0)
(True, 0, False, 1)
(True, 1, 0, False)
(True, 1, False, 0)

Attempt This Online!

@bbayles
Copy link
Copy Markdown
Collaborator Author

bbayles commented May 2, 2025

Yours is the implementation I was endorsing here; I'll change to use it.

@debruijn
Copy link
Copy Markdown
Contributor

debruijn commented May 3, 2025

@debruijn, are you OK with this? The other function implementations are good, but I think this is the core.

Yes, go ahead with just keeping the main function. Also go ahead with swapping it to what you intended with pochmann3s implementation (what you already did) - looking back at my comment I don't understand my response of "Sure, I will change that if you prefer that version over pochmann3's faster implementation, no problem." other than me looking at the wrong comment. Sorry for messing that up, also towards pochmann3.

Edit: I had also included in this comment a suggestion/remark that I now see is not consistent with the interpretation of derangements that will be gone for in this package, so I have deleted it. Please ignore it if you have seen it via email/notifications/etc.

@bbayles bbayles merged commit e922b86 into master May 6, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants