Skip to content
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

Support sequences #342

Merged
merged 33 commits into from
Mar 18, 2022
Merged

Support sequences #342

merged 33 commits into from
Mar 18, 2022

Conversation

hanjinliu
Copy link
Contributor

Trying to add typing supports of such as list[str] and tuple[int, int].
These examples are how it is intended to be used.

@magicgui
def random_2d_image(shape: tuple[int, int] = (100, 100), max: float = 1) -> ImageData:
    return np.random.random(shape) * max
@magicgui
def gaussian_filter(img: ImageData, sigma: list[float] = [1.,]) -> ImageData:
    if len(sigma) == 1:
        sigma = sigma[0]
    return skimage.filters.gaussian(img, sigma)

Still need more strict typing to pass the tests.

@tlambert03
Copy link
Member

i like it! thanks! let me know when you're ready for a full review

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #342 (9c72f7c) into main (1890143) will increase coverage by 0.31%.
The diff coverage is 88.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #342      +/-   ##
==========================================
+ Coverage   88.33%   88.65%   +0.31%     
==========================================
  Files          30       30              
  Lines        3609     3790     +181     
==========================================
+ Hits         3188     3360     +172     
- Misses        421      430       +9     
Impacted Files Coverage Δ
magicgui/widgets/__init__.py 100.00% <ø> (ø)
magicgui/widgets/_concrete.py 85.65% <87.70%> (+1.01%) ⬆️
magicgui/type_map.py 95.59% <100.00%> (+1.44%) ⬆️
magicgui/backends/_qtpy/widgets.py 86.33% <0.00%> (+0.26%) ⬆️
magicgui/widgets/_bases/widget.py 87.84% <0.00%> (+0.55%) ⬆️
magicgui/_type_wrapper.py 67.52% <0.00%> (+2.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1890143...9c72f7c. Read the comment docs.

@hanjinliu
Copy link
Contributor Author

Hi @tlambert03,
I think things are almost done. Could you check my PR?

A few things that I cannot decide by myself are:

  • Should we also support typing like Sequence[int] or Iterable[int]? The new type matcher only detects list[...] and tuple[...] (and their subclasses).
  • For ListEdit, widget options of the child widgets can be set using "options" argument. For instance, options of the int in list[int] is editable by ListEdit(value=[0, 1, 2], options={"max": 10}). But there is no elegant way for TupleEdit since its child widgets could be different (like tuple[int, str]).

@hanjinliu
Copy link
Contributor Author

Sorry, @tlambert03, the last commit is a separated one... (concerning #361)
Can I split them into another PR in some way?

@tlambert03
Copy link
Member

Sorry, @tlambert03, the last commit is a separated one... (concerning https://github.com/napari/magicgui/issues/361)
Can I split them into another PR in some way?

Hey @hanjinliu, sorry for the delay here. I'm ready to take a look at this.

Note that https://github.com/napari/magicgui/pull/362 is solving #361, and a lot of other issues with forward refs. But will likely create conflicts here. Perhaps we should merge that one before coming back to the type_map updates in this PR? I can help with that merge if you'd like.

I'll do a more thorough review once that is resolved, and we can get this in for the next release, which I'd like to do soon.

@hanjinliu
Copy link
Contributor Author

Hi @tlambert03, I've read the upcoming changes in type mapping.
I have no idea how I can pass the "segmentation fault" thing... could you help me solve this?

@hanjinliu
Copy link
Contributor Author

I was misreading a little bit few days ago... but I updated things for the new TypeWrapper.
I hope I'm not doing anything wrong.

@gselzer
Copy link
Contributor

gselzer commented Mar 8, 2022

@hanjinliu this is super awesome! I'd love to use this in my plugin, what is the status of this PR?

@tlambert03
Copy link
Member

what is the status of this PR?

waiting on me/review at this point

@tlambert03
Copy link
Member

so sorry for the delay on this @hanjinliu. been busy and haven't found the time to give this the close look I wanted to. Since it's pretty much just a new feature, (and at first glance looks to be nicely implemented!) I'm going to just merge this so people like @gselzer can take a crack at it. We can iterate as needed if anything arises. Thanks again for your contribution and your patience!

@tlambert03 tlambert03 merged commit 6b95690 into pyapp-kit:main Mar 18, 2022
@tlambert03 tlambert03 added the enhancement New feature or request label Mar 18, 2022
@hanjinliu
Copy link
Contributor Author

Hi, @tlambert03
Thank you for the full review! Yes, I totally agree with that. Let's see how it goes well with plugin development.

@gselzer
Copy link
Contributor

gselzer commented Mar 29, 2022

@hanjinliu @tlambert03 this seems to work great! Thanks for working on this!

This allows us to finally set up variable length arrays and lists in napari-imagej!

Here's an early look at a N-Dimensional gaussian blur with separate dimensional sigmas:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants