Fix missing extent cache invalidation #7015
Conversation
|
I do not think, that we should support modification data in place. I suggest that people should use
All other solution is high risk for performance problem or inconsistency. I do not feel like current code of layers will allow for this. Maby after big refactor. |
I'm not saying we should be automatically firing events (I agree that's too much work we need to inject into arrays and risk having huge performance issues), but I definitely think we should support setting data inplace (and not straight up explode or fail) as long as there's a manual way of letting napari now "Data has changed!". But yeah, we can also encourage people to use |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7015 +/- ##
==========================================
- Coverage 92.78% 92.68% -0.10%
==========================================
Files 620 620
Lines 56779 56781 +2
==========================================
- Hits 52680 52627 -53
- Misses 4099 4154 +55 ☔ View full report in Codecov by Sentry. |
Why is it a bigger performance hit? If you allow modifying in place. Then how it will differ from setting, a new data? |
|
It's a bigger performance hit than just doing |
|
I know, that refresh is less expensive (it is still too expensive - for example trigger recomputation of thumbnail). But I do not see how modify data in place could be less expensive than assign data. |
|
I kinda like the idea of the hard refresh that the user can do? |
👆 I agree wholeheartedly with this. If something is not refreshed by layer.refresh, I consider that a bug. Obviously, in many places we need partial refresh — we can add support for that in other ways. e.g. |
You forget about colormap etc. |
of course. That's a problem with our programming style, though. It does not change the point that as a user, I would definitely expect Anyway, I wouldn't mind some middle ground, where all the attributes that can be refreshed are false by default, but we add |
|
What about something like this in the last commit? Then I would have to change every occurence of |
|
Sorry to be so late to this. Still would be nice to do a "no seriously refresh everything like for real" |
Personally I really like this approach! |
|
Let me know what you think. I went through all the refresh calls and updated it accordingly... it's a bit tricky to understand the control flow of our refreshes sometimes, so it would be great to get other eyes on it. I also expect some tests to fail and let me know where I messed up :P I also left some TODOs where I wasn't sure what to do. |
|
Ok now it's correct. Not sure what I did wrong ^^' |
|
Discussed in community meeting: @Czaki pointed out (and I agree) that cache invalidation is hard 😂 and we would probably benefit with longer testing on main, so we have decided to move it to the next milestone. |
|
(with an idea to merge immediately after the 0.5.1 release) |
|
@jni we didn't merge immediately after 0.5.1.... shoudl we still get this in? |
I think that you should merge this just after 0.5.2 release |
|
Ach. I'll try to remember... 😕 |
|
@jni reminder since you're in release mode and I thought about this! |
|
haha I also thought about this (especially because I think #7146 should also get merged quickly so we can all play with it on main). But your warning comes a few hours too early, and is thus useless! 😂 I have set a reminder on my watch to merge this in the morning though. 😃 |
# References and relevant issues # Description In #7015 the change from cleaning extent to refresh it miss that `_block_refresh` prevents from invalidating cache when a callback connected to event expects it. It also updates pasting spacing information to reduce layer refresh.
References and relevant issues
Started on zulip: https://napari.zulipchat.com/#narrow/stream/212875-general/topic/Layer.2Erefresh.28.29.20doesn't.20reset.20dims.20range
Description
The issue at the core is the fact that we cache extents, and are unable to invalidate this cache when layer data is changed inplace (since we have no events associated with the array inside).
Example failing code:
This should result in dims going from 0 to 6, with alternating empty slices. Instead, we get 0-3, with only empty slices, cause the points layer extents are outdated.
Adding the following lines will fix the issue:
For now, I only switched from
refresh()to_clear_extents_and_refresh()where we do have knowledge of a change in data. However, to actually fix the above code, we need to somehow either:refresh()for non array-like layers (might be a big hit on performance cause we refresh in a lot of places that don't really need thisrefreshto always invalidate cahce, but expose a "soft refresh" that does not, and use that throughout napari every time we don't need cache invalidationI think 3. is the best, but it's also a bit more invasive and I'm not sure how many people use refresh and rely on it being performant outside of napari core.