Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-98169 dataclasses.astuple support DefaultDict#98170

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
carljm merged 5 commits intopython:mainfromkwsp:dataclass
Mar 13, 2023

Conversation

kwsp
Copy link
Contributor

@kwspkwsp commentedOct 11, 2022•
edited
Loading

Basically applying the same fix forasdict from#32056 toastuple. These two should've been the same PR, but I didn't know aboutastuple previously.

Also modified the handling ofDefaultDict inasdict a little so that we only callisinstance(obj, dict) once. This results in a ~3% speed up on my machine on this example:

# benchmarkfromdataclassesimportdataclass,asdictfromcollectionsimportdefaultdictfromtypingimportDefaultDict,Dict,List,NamedTupleimporttimeclassNT(NamedTuple):x:inty:float@dataclassclassC:dd:DefaultDict[str,List]nt:NTd1:Dict[str,List]d2:Dict[str,int]defget_instance():instance=C(dd=defaultdict(list),nt=NT(1,1.),d1={},d2={})instance.dd["x"].append(12)instance.dd["x"].append(13)instance.d1["y"]= [1,2,3]instance.d2["z"]=1returninstanceinstance=get_instance()deftimeit(callable,n_runs=100_000,name=""):t1=time.perf_counter_ns()for_inrange(n_runs):callable()elapsed_ms=round(1e-6* (time.perf_counter_ns()-t1),2)print(f"{name}\t{n_runs} runs:{elapsed_ms} ms")timeit(lambda:asdict(instance),name="asdict")timeit(lambda:astuple(instance),name="astuple")

@eendebakpt
Copy link
Contributor

@kwsp Can you address the review comments?

cache the output of `type(obj)` to avoid computing it 3 times.Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@kwsp
Copy link
ContributorAuthor

@eendebakpt sorry I missed the notification. Addressed your change.

Could you please comment on my response toif instanceof(obj, defaultdict)?

@eendebakpt
Copy link
Contributor

@eendebakpt sorry I missed the notification. Addressed your change.

Could you please comment on my response toif instanceof(obj, defaultdict)?

@kwsp I cannot see the comment

Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@kwsp
Copy link
ContributorAuthor

#98170 (comment)

I responded:

Good suggestion. I did this originally to be consistent with the existing code that checks for named tuples, and to avoid importing thecolelctions module.

elifisinstance(obj,tuple)andhasattr(obj,'_fields'):

I originally assumed that for thenamedtuple case, they could've done ais instance(obj, namedtuple) but didn't to avoid an import fromfrom collections import namedtuple, but I just realisedis instance(obj, namedtuple) wouldn't work becausenamedtuple is a factory function...

So now the question is should we importcollections in this module? I assume it's imported elsewhere during Python startup so adding the import here should be free right?

@eendebakpt
Copy link
Contributor

I do not now the origins of thehasattr check, but I agree the import time ofcollections should not be an issue. But let's keep the code consistent for now and keep thehasattr just as in theasdict code.

Copy link
Member

@carljmcarljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

IMO it would probably be better to just doisinstance(obj, defaultdict) in both of these cases, but I also think that's a bit out of scope for this PR. The code here LGTM, seems like a straightforward extension of the feature that was already merged forasdict.

kwsp reacted with thumbs up emoji
@carljmcarljm merged commit71e37d9 intopython:mainMar 13, 2023
carljm added a commit to carljm/cpython that referenced this pull requestMar 14, 2023
* main: (50 commits)pythongh-102674: Remove _specialization_stats from Lib/opcode.py (python#102685)pythongh-102660: Handle m_copy Specially for the sys and builtins Modules (pythongh-102661)pythongh-102354: change python3 to python in docs examples (python#102696)pythongh-81057: Add a CI Check for New Unsupported C Global Variables (pythongh-102506)pythonGH-94851: check unicode consistency of static strings in debug mode (python#102684)pythongh-100315: clarification to `__slots__` docs. (python#102621)pythonGH-100227: cleanup initialization of global interned dict (python#102682)  doc: Remove a duplicate 'versionchanged' in library/asyncio-task (pythongh-102677)pythongh-102013: Add PyUnstable_GC_VisitObjects (python#102014)pythonGH-102670: Use sumprod() to simplify, speed up, and improve accuracy of statistics functions (pythonGH-102649)pythongh-102627: Replace address pointing toward malicious web page (python#102630)pythongh-98831: Use DECREF_INPUTS() more (python#102409)pythongh-101659: Avoid Allocation for Shared Exceptions in the _xxsubinterpreters Module (pythongh-102659)pythongh-101524: Fix the ChannelID tp_name (pythongh-102655)pythongh-102069: Fix `__weakref__` descriptor generation for custom dataclasses (python#102075)pythongh-98169 dataclasses.astuple support DefaultDict (python#98170)pythongh-102650: Remove duplicate include directives from multiple source files (python#102651)pythonGH-100987: Don't cache references to the names and consts array in `_PyEval_EvalFrameDefault`. (python#102640)pythongh-87092: refactor assemble() to a number of separate functions, which do not need the compiler struct (python#102562)pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102631)  ...
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull requestMar 27, 2023
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this pull requestApr 11, 2023
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@carljmcarljmcarljm approved these changes

@eendebakpteendebakpteendebakpt approved these changes

@ericvsmithericvsmithAwaiting requested review from ericvsmithericvsmith is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@kwsp@eendebakpt@carljm@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp