GeezerVison is showing github.com — gh-119127: functools.partial placeholders by dg-pb · Pull Request #119303 · python/cpython
Conversation
dg-pb
commented
May 21, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Could you add some tests? Or is that pointless without approval?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I was thinking to wait for approval first.
|
nineteendo reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
dg-pb
changed the title
gn-119127: functools.partial placeholders
gh-119127: functools.partial placeholders
May 21, 2024
|
If this was accepted, the implementation is ready for review. Python and C versions are in sync and tests implemented.
|
nineteendo reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
|
Please open a new clean PR against main. Make sure the diff against main is correct before opening the PR.
|
nineteendo reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
rhettinger
Awaiting requested review from rhettinger
rhettinger is a code owner
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|