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

Replace use of properties with features #425

Merged
merged 16 commits into from
Jun 17, 2024

Conversation

andy-sweet
Copy link
Member

@andy-sweet andy-sweet commented Jun 10, 2024

References and relevant issues

Part of napari/napari#3911

Description

This replaces use of properties, property_choices, and current_properties with features and feature_defaults. The motivation for doing this now is to make the latest documentation consistent with existing code and examples on main (here and on napari/napari) for the upcoming v0.5 release. As we also plan to deprecate properties in v0.5 (napari/napari#3911), this documentation update is necessary for that.

@andy-sweet andy-sweet added this to the 0.5.0 milestone Jun 10, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 10, 2024
@jni
Copy link
Member

jni commented Jun 11, 2024

Cool, thanks @andy-sweet! Let me know when you think this is ready for review! 🙏

@@ -121,66 +121,6 @@ napari.run()
* Graph - check this box to display a previously created graph as explained in
[](#arguments-of-view_tracks-and-add_tracks).

## Arguments of `view_tracks` and `add_tracks`
Copy link
Member Author

Choose a reason for hiding this comment

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

These docs are incomplete and stale for multiple reasons (including properties/features), so I removed this in favor of link to the two functions in the text above.

Copy link
Member

Choose a reason for hiding this comment

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

lmao I came here while trying to resolve merge conflicts with #420 going, "why on earth did this get removed. And no discussion either, wth!"

... Then I found my thumbs-up above 🤣

@@ -4,8 +4,8 @@

```{Admonition} DEPRECATED ATTRIBUTES
:class: warning
As of napari 0.5.0, `edge_*` attributes are being renamed to
`border_*` attributes. We have yet to update the images and/or videos in
As of napari 0.5.0, `edge_*` attributes are being renamed to
Copy link
Member Author

@andy-sweet andy-sweet Jun 11, 2024

Choose a reason for hiding this comment

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

The changes in this file mostly come from the corresponding PR over on the pointannotator repo: kevinyamauchi/PointAnnotator#6

I tested the example on that repo with napari's main.

There are also some other name swaps.

@@ -100,16 +100,16 @@ def circularity(perimeter, area):
image = data.coins()[50:-50, 50:-50]
label_image = segment(image)

# create the properties dictionary
properties = regionprops_table(
# create the features dictionary
Copy link
Member Author

Choose a reason for hiding this comment

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

The code changes come from napari's examples/annotate_segmentation_with_text.py example and should be consistent now.

Other changes in this file are mostly word swaps.

@andy-sweet andy-sweet marked this pull request as ready for review June 11, 2024 22:52
@andy-sweet
Copy link
Member Author

@jni: I have one last compatibility check to do with the btrack cell-tracking example, but other this is ready for review now.

@andy-sweet andy-sweet changed the title [WIP] Replace use of properties with features Replace use of properties with features Jun 12, 2024
@andy-sweet
Copy link
Member Author

@jni: I have one last compatibility check to do with the btrack cell-tracking example, but other this is ready for review now.

Check is done.

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Other than the specific comment below, I think there's a general overlap between "table" and "dictionary" which might be confusing to new users.

Comment on lines 440 to 442
dictionary with two feature columns: `good_point` and `confidence`.
The table has three rows since there were three coordinates provided in `points`.
We set the face color as a function of the `confidence` feature by providing the
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a bit confusing for folks unfamiliar with pandas to talk about columns/rows in a dictionary.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, indeed, maybe worth saying that this is converted internally to a data frame with two columns and three rows? (I wouldn't specify pandas necessarily, but maybe.)

Copy link
Member Author

@andy-sweet andy-sweet Jun 13, 2024

Choose a reason for hiding this comment

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

Agreed this paragraph is mixing things a bit too much. Let me try to fix that.

But I also want to make a distinction between the meaning of features (i.e. it's a table) and the type used to provide or implement it (i.e. a dict or DataFrame). Ideally, we could do that in one place for all layers (see #23).

For now, there's a little effort above to describe what the table is in the "### Using the points features table" section. I'll clean that up a little and will mention that the table can be provided as a dictionary where the values are array-likes of the same length.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also (tried to) put a link back to the section that describes the dictionary and table/dataframe relationship in both of these similar paragraphs.

@@ -355,7 +355,7 @@ text_parameters = {
}
```

The `text` key specifies pattern for the text to be displayed. If `text` is set to the name of a `properties` key, the value for that property will be displayed. napari text also accepts f-string-like syntax, as used here. napari will substitute each pair of curly braces(`{}`) with the values from the property specified inside of the curley braces. For numbers, the precision can be specified in the same style as f-strings. Additionally, napari recognizes standard special characters such as `\n` for new line.
The `string` key specifies pattern for the text to be displayed. If `string` is set to the name of a `feature` column, the value for that feature will be displayed. napari text also accepts f-string-like syntax, as used here. napari will substitute each pair of curly braces(`{}`) with the values from the feature specified inside of the curly braces. For numbers, the precision can be specified in the same style as f-strings. Additionally, napari recognizes standard special characters such as `\n` for new line.
Copy link
Member

Choose a reason for hiding this comment

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

I always forget about this. It's so cool. 😃

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Thanks @andy-sweet, this is excellent! I think @brisvag's minor comment is worth addressing, but I'm also ok with it happening later, so I'm gonna approve this and ready-to-merge it, and if you get to it great and if not we can make an issue. 😂

@andy-sweet
Copy link
Member Author

I think I addressed the feedback and also fixed a couple of links/cross-references. Please take another quick look before merging!

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Love the changes :)

@andy-sweet
Copy link
Member Author

Thanks for the second look. I'll merge this after the weekend unless anyone objects.

@andy-sweet andy-sweet merged commit 1dd9162 into napari:main Jun 17, 2024
8 checks passed
@andy-sweet andy-sweet deleted the deprecate-properties branch June 17, 2024 15:37
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/error-while-adding-text-to-napari/76960/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants