-
Notifications
You must be signed in to change notification settings - Fork 74
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
improve laggy subset finalization #3175
improve laggy subset finalization #3175
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3175 +/- ##
==========================================
- Coverage 88.42% 88.37% -0.05%
==========================================
Files 122 122
Lines 18267 18278 +11
==========================================
+ Hits 16153 16154 +1
- Misses 2114 2124 +10 ☔ View full report in Codecov by Sentry. |
56d5bb8
to
0e9c1ca
Compare
2191ae0
to
7b6b95d
Compare
child_layer_icons[child_layer] = f'{parent_icon}{i}' | ||
if child_layer not in self.state.layer_icons: | ||
child_layer_icons[child_layer] = f'{parent_icon}{i}' | ||
|
||
self.state.layer_icons = { | ||
**self.state.layer_icons, | ||
**child_layer_icons | ||
} | ||
if child_layer_icons: | ||
self.state.layer_icons = { | ||
**self.state.layer_icons, | ||
**child_layer_icons | ||
} |
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.
FYI: @bmorris3 - does this seem reasonable? I'm trying to avoid an update to the state again if no new child layers are added.
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 think I notice a marginal improvement on my windows PC and the code looks reasonable so I will approve. Looking forward to more optimization in this direction!
I found this to be a very minor improvement, but this apparently isn't the source of most of the (millions of) calls to |
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.
Approved, but please don't close until @pllim has remade the deleted corresponding issue.
I won't have this close #3181 since it doesn't fully address the lag (although it should address the unnecessary calls to spectral extraction during the subset creation process). |
* only call subset_selected_changed_callback for matching subset updates * optimize handling subset_state changes * prevent unnecessary state-update * avoid (some cases of) startswith since can be expensive on some machines
Description
This pull request limits the updates to
subset_selected_changed_callback
(originally introduced in #2939) to only trigger for subset update messages corresponding to the currently selected subset (and only when the subset state has been changed, not color, etc) and also minimizes spectral extraction calls whensubset_select.items
are changed to only affect the selected entry.Fixes #3164Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.