-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: add category #239
feat: add category #239
Conversation
Codecov Report
@@ Coverage Diff @@
## main #239 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 37 37
Lines 2785 2801 +16
=========================================
+ Hits 2785 2801 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@DragaDoncila and @nclack ... I wish i didn't have to request reviews from folks who also work on/with the hub team, since those review requests unfortunately don't get reciprocated in the other direction. but you're the folks who are paying attention to this repo. thank |
In a followup pr, we should add a less constrained field (like "keywords") that could be any string. if desired, people could use it for the EDAM labels, i.e. |
I feel like it would be better to associate the category with a command instead of the manifest. I think this is what vscode does? And it makes sense to me that I could have a "Segmentation" command and a "Visualization" command, and I'd want to be able to search for them separately.
This is probably the main thing I'm worried about here - tying the npe2 schema to a particular set of categories. I want it to be a string/list-of-string here with the salient categories determined by the use case on the napari side. |
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.
Interesting! and nice point to raise @nclack - I think it's worth keeping the 'plugin category' separate from the 'command category' concept, many plugins may implement a few commands for denoising but I likely wouldn't choose this over a specialist denoising plugin when choosing which plugin to download.
I want it to be a string/list-of-string here with the salient categories determined by the use case on the napari side.
This makes sense to me, my only concern is that without providing a list of pre-defined categories in the spec we're likely to end up in a situation where the 'categories' users provide are less consistent (and therefore less useful) what do you think @tlambert03 ?
Given that this is proposed with a very specific UI goal in mind and I agree that that UI goal is better served by plugin categories than command categories I'm approving here
vscode uses a relatively small, fixed set of categories: I think the trick is being broad enough that we don't need dozens of categories, while being specific enough to be useful. I also don't want to restrict the possible scope here to BioImaging.
I do get that concern, but share @alisterburt's thoughts that without having a predefined set of choices, it could go off the rails. The trick of course is in the selection, so would appreciate your thoughts on the current list, copied below for convenience (and we can add more in the future, but shouldn't go nuts). I came to this selection by looking through all the current plugins, and by looking a bit through Fiji (though again, don't want to restrict ourselves to that scope ultimately).
another field added later could be less constricted (such as |
Co-authored-by: alisterburt <alisterburt@gmail.com>
one more clarification: a plugin that offered both segmentation and visualization routines would just add both strings to their categories. Users filtering based on either would see that plugin. Then, at runtime (once in napari) we can do much more with command-specific searching. I'm not sure if feels right here to extend the command schema for this (i feel like it would lead to a tremendous amount of verbosity ... imagine |
Thanks! I missed that. Will keep the discussion focused on plugin categories! When I'm looking at the vscode extension categories, I think the goal is that the use is pretty well scoped so they don't evolve (much).
I like the intent. How confident are you feeling about the categories in the pr at the moment? One of the most common vscode categories is "Other" but this pr doesn't have that.
I'm more or less wondering where they should be validated. As long as there's validation somewhere, I imagine we'll be able to keep the set of categories well defined. Also, I'm not sure how to define a minimally sufficient set of categories and so I'd like to punt the decision to somewhere closer to the use case. One can't punt forever though and the vscode examples give me a pretty good idea of what to shoot for. So if I can't convince on the "move the decision toward the use-case" front (and it sounds like I can't 😄), I can review the proposed categories as is. |
totally agree.
maybe 80%? 😂 I feel like they represent mostly non-overlapping applications that cover a broad range... but I also think it's likely that at least one more thing would need to be added. would definitely like more eyes on it (cc @jni, @sofroniewn, @kevinyamauchi, @DragaDoncila )
Yeah, I noticed that too. I could add an "other" ... but I struggled to see the difference/merit between
Yes, they're validated. by using an Enum, pydantic casts strings and fails if they are not one of the enumerations members.
I'm happy to be moved! I'm not really following what the suggestion is though? The goal was definitely to try to pick categories that largely cover existing plugin use cases, while not restricting future cases or being to bio-focused. |
Other just makes sure every plugin can be assigned at least one category.
I meant will This would mean that The more I write, the more I think I'm basically advocating for going off the rails (unstructured tagging). And I agree that's not so useful. |
yes... but why? 😂 internally, what's the difference between |
oh, I see. like napari would maintain a list, and npe2 couldn't validate them without napari? yeah, I think my preference is to have them over here. |
Honestly I really like it. I think having an The only real question for me is if we think any of these categories would ever be removed, and as I look through them I think the answer is "probably not". If we do think some of them should be removed, then I suggest we err on the side of removing, since we can easily add categories back in. I can imagine having to expand the list to continue getting meaningful filtering as the number of plugins grows, and perhaps also adding a hierarchy? A hierarchy would probably be the most annoying to make backwards compatible but it would definitely be doable, and potentially many of these categories would end up as roots of the hierarchy anyway. Definitely would like to see the addition of keywords as general tags as well in a future PR - tags are the |
agree completely, we should trim anything we're not completely happy with. Here are the ones that I'm less sure about, either in terms of inclusion or wording:
|
also very much agree. Hierarchy's could be colon separated, so i guess another critical look at the existing proposal from the perspective of "are these are roots" would be good |
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, @tlambert03 ! Overall, this looks good to me.
From my perspective, the categories should all be at the same level in a hiearchy (i.e., none of the categories are a subset of another). In that lens, I find the inclusion of "Image Processing" and "Denoising" a bit confusing. It feels like "denoising" is a bit specific relative to a category like "Image Processing" and "Denoising" feels like an "Image Processing" operation. What types of plugins did have in mind for "Image Processing"?
I am wondering if "Denoising" and "Image Processing" could be removed and replaced with "filter" or "filtering" (or "Image Filtering"?)?
I don't feel super strongly about this, so happy to defer to the group.
edit: I just read the comment below regarding "roots". I don't think "Image Processing" would be at the same level of hierarchy as "Denoising". I could imagine some of the other categories would be children of "Image Processing" as well (e.g., "Segmentation", "Transformations"). I'm not sure if it makes sense to make "Image Processing" root or start at the level of more specific "Image Processing" operations (e.g., filtering, segmentation, denoising)
so i guess another critical look at the existing proposal from the perspective of "are these are roots" would be good
edit 2: actually after some thought, I think it will yield more usable search results for users if the root starts more granular than "image processing". Thus, it seems to me that we should eliminate "Image Processing" and replace it some more specific categories (e.g., "image filtering").
Some feedback from the August 17, 2022 community meeting:
|
thanks @kevinyamauchi!
yep, I can see that. as commented in #239 (comment) ... I think denoising is perhaps too specific. lets remove for now.
well, scikit-image would essentially be Image Processing, but not (just) filtering, and not (just) transforms, etc...
Yep, that one really does seem to be the most overlap-prone. Let's definitely kill "denoising", and then would love to get @jni to weigh in on the rest. My thoughts: yes, segmentation generally involves some sort of image processing, but there are so many segmentation plugins, that grouping them all as "image processing" seems to vague. on the other hand, swapping "image processing" with the much more specific "filtering", means that any image processing that is not filtering doesn't really have a place. So that is indeed the trick here. For example, if we kill "image processing" do we also add "morphological operations", (which are clearly not filters) etc...? It seems a bit easier to later add
we can remove it if you'd like, but i don't think we should be more specific yet |
one sentence summaries:
|
Thanks for the detailed reply, @tlambert03 ! The one sentence summaries really helped clarify things for me.
I see your point. Given the diversity of
Sorry, I think I was a bit terse in my summary (rushing off to dinner). There was a bit more nuance here in that people felt that while |
Once we sort the question regarding |
Also, <3 for including simulation! I think napari has a ton of potential as a front end for simulation. |
A plugin needn't have a single category, it's a list, so you could definitely have I don't see that as violating "rootness" as you could also just have This isnt however a field for dependencies, so it shouldn't be "tensor flow" or "PyTorch" for example (we already have that in dependencies), and I would argue it also shouldn't be "GAN" or "U-Net" either (too specific). So, it's either remove it or keep it. Thoughts? |
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.
This looks good to me! Thank you for your patient explanations, @tlambert03 . Based on the discussion, I would prefer that we remove denoising
and keep Machine_learning
.
Co-authored-by: Kevin Yamauchi <kevin.yamauchi@gmail.com>
thanks @kevinyamauchi. will let this sit for another few days if anyone else wants to comment |
I agree with most of what came up so far, great summary!
Does this mean that something like |
but again, as mentioned above, it doesn't need to be just machine learning. i.e. |
Sorry, I put my answer a bit backwards above; when I said:
I meant it as an answer to your
I'm not saying I'm opposed to |
Indeed, that's a very good way to put it. And perhaps a reason to hold off on including it, if only for being an outlier in that regard |
removed, prolly easier to add than remove |
this has been up for a long time, and received a lot of input. So merging. thanks all |
This reverts part of #38 and adds back a category field with a defined list of allowable values for use in in-app filtering.
see #239 (comment) below for clearer detail