Repository navigation
gh-119127: functools.partial placeholders - #119303
Closed
dg-pb wants to merge 119 commits into
Closed
dg-pb wants to merge 119 commits into
dg-pb wants to merge 119 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
nineteendo
reviewed
May 21, 2024
Comment thread
Modules/_functoolsmodule.c
Outdated
Copy link
Copy Markdown
Contributor
|
Could you add some tests? Or is that pointless without approval? |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
I was thinking to wait for approval first. |
Sorry, something went wrong.
This comment was marked as outdated.
Copy link
Copy Markdown
Contributor
Author
|
If this was accepted, the implementation is ready for review. Python and C versions are in sync and tests implemented. |
Sorry, something went wrong.
PR python#119321 added a comment about the magic number bump but did not actually apply the new magic number.
…ython#119492) ``_fancy_replace()`` is no longer recursive. and a single call does a worst-case linear number of ratio() computations instead of quadratic. This renders toothless a universe of pathological cases. Some inputs may produce different output, but that's rare, and I didn't find a case where the final diff appeared to be of materially worse quality. To the contrary, by refusing to even consider synching on lines "far apart", there was more easy-to-digest locality in the output.
erlend-aasland
removed request for
a team,
Fidget-Spinner,
abalkin,
ambv,
barneygale,
brettcannon,
corona10,
ericsnowcurrently,
ericvsmith,
erlend-aasland,
ethanfurman,
ezio-melotti,
freakboy3742,
gpshead,
hugovk,
iritkatriel,
isidentical,
jaraco,
kumaraditya303,
markshannon,
ncoghlan,
pganssle,
serhiy-storchaka,
tiran,
vsajip and
warsaw
May 31, 2024 07:39
Copy link
Copy Markdown
Contributor
|
Please open a new clean PR against main. Make sure the diff against main is correct before opening the PR. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Thanks Erlend. |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Assignees
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As I already had implementation I though PR might be helpful for others to see and evaluate.
From all the different extensions of functools.partial I think this one is the best. It is relatively simple and exposes all missing functionality. Other partial extensions that I have seen lack functionality and would not provide complete argument ordering capabilities and/or are too complicated in relation to what they offer.
Implementation can be summarised as follows:
a) All trailing placeholders are removed. (Makes things simpler)
b) Throws exception if not all placeholders are filled on call
c) retains optimization benefits of application on other partial instances.
Performance penalty compared to current functools.partial is minimal for extension class. + 20-30 ns for initialisation and <4 ns when called with or without placeholders.
To put it simply, new functionality extends functools.partial so that it has flexibility of lambda / def approach (in terms of argument ordering), but call overhead is 2x smaller.
The way I see it is that this could only be justified if this extension provided completeness and no new functionality is going to be needed anywhere near in the future. I have thought about it and tried various alternatives and I think there is a good chance that this is the case. Personally, I don't think I would ever need anything more from partial class.
Current implementation functions reliably.