-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
♻️ avoid deepcopy of dict in validate_coerce #3946
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
Conversation
|
|
||
| if "type" in v_copy: | ||
| trace_type = v_copy.pop("type") | ||
| elif isinstance(v_el, Histogram2dcontour): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this check never hits as in in line 2647 is checked whether v_el is a dict.
|
Thanks for this PR! Sorry I haven't had a chance to review it in detail yet, but the performance gains seem promising! |
|
Hi @nicolaskruchten, No worries, we both know how busy times can get! Cheers, |
Co-authored-by: Alex Johnson <[email protected]>
alexcjohnson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 Thanks very much @jvdd, and apologies for the delay!
This PR avoids the
deepcopyin the_plotly_utils.basevalidators.BaseDataValidatoritsvalidate_coercemethod.The reason I updated this code; I profiled plotly.py its graph construction time when adding large traces and +/- 70% of the time was spent in this
validate_coercemethod - of which +/- 33% of the time was spent at thedeepcopy(see below).I believe it is unnecessary overhead to first create a dict (with
.to_plotly_json) and then make adeepcopyof that dict to possiblypopone key (even whenv_elis a dict and not aBaseTraceType, this still seems like unnecessary overhead to me).Would appreciate any remarks!
Perhaps it might also be interesting to try avoid the.to_plotly_jsoncall when it is aBaseTraceTtype- as this also takes a considerable amount of time (for large traces)?-> @jonasvdd took a look at this and replaced the
.to_plotly_json(which callsdeepcopyon theself._props) with justself._props)What I profiled:
Before & after this PR: