Skip to content

Conversation

@tlambert03
Copy link
Member

part of a fix to #541, this makes it so that when you use register_type(Union[...], return_callback=...), then you effectively register the callback for functions that return either the union or any type within the Union (not just the actual union object itself)

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage: 92.00% and project coverage change: +0.09 🎉

Comparison is base (c5f7a13) 89.66% compared to head (3d64e5f) 89.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   89.66%   89.75%   +0.09%     
==========================================
  Files          37       37              
  Lines        4423     4433      +10     
==========================================
+ Hits         3966     3979      +13     
+ Misses        457      454       -3     
Impacted Files Coverage Δ
src/magicgui/type_map/_type_map.py 96.41% <92.00%> (+0.16%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tlambert03
Copy link
Member Author

cc @Czaki, if you have a moment to review, this implements option 3 from #541 (comment)

@tlambert03 tlambert03 requested a review from Czaki March 10, 2023 14:35
@tlambert03 tlambert03 changed the title Register all types in a return_callback Union feat: Register all types in a Union when passed to return_callback Mar 10, 2023
Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

I do not see documentation update, but I assume that it is connected with plan to remove return callback?

Also, in the context of napari:

Currently problem is solved in this way (main only, not released):
https://github.com/napari/napari/blob/9e911040148d232b76d768ed74ff90ac0fa6f707/napari/types.py#L145
https://github.com/napari/napari/blob/9e911040148d232b76d768ed74ff90ac0fa6f707/napari/types.py#L158

But code in magicgui does not contain deduplication of callbacks.

So I understand that After merging it there should be added if to napari codebase to check it depends on the magicgui version?

@tlambert03
Copy link
Member Author

So I understand that After merging it there should be added if to napari codebase to check it depends on the magicgui version?

yeah, I can do that PR if you'd like?

@tlambert03
Copy link
Member Author

updated the docs a bit. thanks for the review @Czaki !

@tlambert03
Copy link
Member Author

But code in magicgui does not contain deduplication of callbacks.

this is a good point... but i think in most cases it's going to be fine (the second one will overwrite the first one). let me double check that for you

@tlambert03 tlambert03 merged commit 50028f3 into pyapp-kit:main Mar 11, 2023
@tlambert03 tlambert03 deleted the fix-return-optional branch March 11, 2023 18:44
@Czaki
Copy link
Contributor

Czaki commented Mar 11, 2023

this is a good point... but i think in most cases it's going to be fine (the second one will overwrite the first one). let me double check that for you

Yes and the whole recalculation of all attributes and double transfer to GPU memory of layer data?

@tlambert03
Copy link
Member Author

tlambert03 commented Mar 11, 2023

Yes and the whole recalculation of all attributes and double transfer to GPU memory of layer data?

no, I don't mean the second "layer" will overwrite the first "layer", I mean the second registered callback will overwrite the first registered callback (in the type2callback map) and only a single one will actually be called at runtime

@tlambert03
Copy link
Member Author

buuut nope... it gets called twice. will fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants