Line plot widget from layer Metadata for plugins #63
Conversation
|
I'm 👍 for including this. It looks like napari layers have a |
|
Hi @dstansby , I would be OK to change from |
|
That makes sense, I didn't realise |
| # opacity value for the lines | ||
| _line_alpha = 0.5 |
There was a problem hiding this comment.
To keep things simple I think we should just use the default Matplotlib settings, so remove this custom alpha setting.
| class Line2DBaseWidget(NapariMPLWidget): | ||
| # opacity value for the lines | ||
| _line_alpha = 0.5 | ||
| _lines = [] |
There was a problem hiding this comment.
What's the motivation for saving a list of lines as a class attribute?
| def __init__(self, napari_viewer: napari.viewer.Viewer): | ||
| super().__init__(napari_viewer) | ||
|
|
||
| self.axes = self.canvas.figure.subplots() |
There was a problem hiding this comment.
The easier and recommended (because it sets up the right theme for the axes) way to do this - I think this is new since this PR was opened.
| self.axes = self.canvas.figure.subplots() | |
| self.add_single_axes() |
| super().__init__(napari_viewer) | ||
|
|
||
| self.axes = self.canvas.figure.subplots() | ||
| self.update_layers(None) |
There was a problem hiding this comment.
I'm not sure this needs to be here?
| def __init__(self, napari_viewer: napari.viewer.Viewer): | ||
| super().__init__(napari_viewer) | ||
| self.setMinimumSize(200, 200) | ||
| self._plugin_name_widget = magicgui( |
There was a problem hiding this comment.
I'm confused about what the plugin_name stuff is for - could you explain?
There was a problem hiding this comment.
This was because I thought about a scenario where there are multiple plugins saving/loading settings/features in a layer metadata. If these features have the same name, there would be overwriting problems. That is also why I started with a nested dictionary, as a way to isolate them. What do you think?
We could also drop this level, it would be more up to the user to know that this can happen.
| if len(self.layers) == 0: | ||
| return [] | ||
| else: | ||
| metadata = self.layers[0].metadata |
There was a problem hiding this comment.
Would it be possible to get the metadata from all the layers? That way if more than one layer was selected you could choose from metadata in any of the selected layers.
There was a problem hiding this comment.
That is a good idea!
|
|
||
| def _get_valid_metadata_keys( | ||
| self) -> List[str]: | ||
| """Get metadata keys if nested dictionaries""" |
There was a problem hiding this comment.
What's the motivation for supporting nested dicts here? I think we should try and make the implementation as simple as possible to start with, which I think means just supporting items in the metadata dictionary that have string keywords and np.ndarray values.
There was a problem hiding this comment.
This is related to the plugin_name question here.
|
Hi @dstansby , Thanks for the review and sorry for the long delay! I can restart working on this next week, I am leaving a few comments now. Recently, a line plot widget from the features property has shown more useful to me (despite not working with the image layer), similar to the already existing FeaturesScatterWidget. Thus, I intend to add it as a second class here as well. Maybe both can be just one, not sure yet if that's a good idea. |
Co-authored-by: David Stansby <[email protected]>
|
Stepping back a bit, is there a specified structure for the |
Yeah as I said recently, I was working with metadata in order to get features stored in an Image layer, thus the metadata, but I am leaning towards Labels layer anyway. I am refactoring this code to make line plots for the Labels layer. |
|
👍 closing and opening a new PR is good for me |
|
Closing this as #184 was just opened and should replace this. |
Hi @dstansby, hi everyone,
I am leaving my suggested changes for this new feature in this PR. This implementation would fullfil #60 .
I don't necessarily expect this to be reviewed soon, so please take your time! :)
On the other hand, please do let me know as soon as possible if these changes can be implemented here so that I can update other repositories that rely on this feature. I am open for changes and feedback!
Best,
Marcelo