Add missing datasets to public API #4951

hmaarrfk · 2020-08-30T22:39:10Z

Description

@scikit-image/core I noticed a few datasets were missing from data.__init__.py.

I added them and updated the examples that used them.

However, I am unclear about the exact meaning of these datasets, and their importance.

Help is appreciated elaborating on the description of each dataset.

TODO:

Provide correct context Add missing datasets to public API #4951 (comment)
data called cells3d
data called mitosis

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.

doc/examples/applications/plot_3d_image_processing.py

skimage/data/__init__.py

alexdesiqueira · 2020-08-30T23:38:10Z

However, I am unclear about the exact meaning of these datasets, and their importance.
Help is appreciated elaborating on the description of each dataset.

Not sure what you mean here @hmaarrfk. Where would we describe? What would you like to see?
There are some descriptions on our GitLab data repo. Is this useful, somehow?

alexdesiqueira · 2020-08-30T23:39:15Z

skimage/data/__init__.py

+def cells3D():
+    """3D microscopy images of cells.
+
+    The data for this was provided by the Allen Institute for Cell Science.  It


I think information like 598–609 should be in a Notes section.

Understanding your data is critical to good analysis and visualization.

What is the difference between the "Notes" and the "Description".

If you feel strongly about it, do reorganize.

We follow the Numpy docstyle. According to them:

The sections of a function’s docstring are:
1. Short summary
A one-line summary that does not use variable names or the function name (...)
(...)
13. Notes
An optional section that provides additional information about the code, possibly including a discussion (...)

I've made some modifications, however, I will point to:

Extended Summary

A few sentences giving an extended description. This section should be used to clarify functionality, not to discuss implementation detail or background theory, which should rather be explored in the Notes section below. You may refer to the parameters and the function name, but parameter descriptions still belong in the Parameters section.

Functionality here, for me, represents things like metadata, depicting voxel size, which are simply not communicated in a numpy array. I've kept those details.

hmaarrfk · 2020-08-31T01:18:45Z

Thanks for the review @alexdesiqueira! I don't see the need for a Notes section if the description is empty.

IMO: we can reorganize once the description gets crowded.

pep8speaks · 2020-08-31T04:53:02Z

Hello @hmaarrfk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/data/_registry.py:

Line 134:80: E501 line too long (91 > 79 characters)
Line 138:80: E501 line too long (88 > 79 characters)
Line 143:80: E501 line too long (88 > 79 characters)
Line 148:80: E501 line too long (158 > 79 characters)
Line 149:80: E501 line too long (132 > 79 characters)

Comment last updated at 2020-10-29 08:13:43 UTC

sciunto · 2020-08-31T04:53:33Z

CC @mkcor

sciunto · 2020-08-31T06:42:23Z

doc/examples/applications/plot_3d_image_processing.py



 #####################################################################
 # Load and display 3D images
-# ==========================
-# Three-dimensional data can be loaded with `io.imread`.
+#


Suggested change

#

# ==========================

sciunto · 2020-08-31T06:43:12Z

doc/examples/applications/plot_3d_image_processing.py

-# Three-dimensional data can be loaded with `io.imread`.
+#
+# The optional dependency, pooch is required to obtain the dataset.
+#####################################################################


This line is not necessary.

sciunto · 2020-08-31T06:44:04Z

skimage/data/__init__.py

+
+    Notes
+    -----
+


This blank line can be deleted.

skimage/data/__init__.py

sciunto · 2020-08-31T06:48:54Z

I think my comments will turn the CI green. Once applied, i'm ready to approve.

jni

Approved with @sciunto's suggestions.

skimage/data/tests/test_data.py

skimage/data/__init__.py

hmaarrfk · 2020-08-31T12:05:27Z

Thank you all for the reviews.

@mkcor suggested that we unify the names in the registry with the names of the functions.

Now that I look at this, I notice that we might be "breaking" a private API. Are you guys OK with that? I'm not sure how many people you've pointed to these functions.

mkcor · 2020-08-31T13:00:27Z

@mkcor suggested that we unify the names in the registry with the names of the functions.

Did I? ;-)

Are you referring to my considerations about one-word names?

hmaarrfk · 2020-08-31T13:01:28Z

I'm refering to #4951 (comment) :D

mkcor · 2020-08-31T13:10:02Z

I'm refering to #4951 (comment) :D

Oh! Well, if this PR is merged, fetch('data/cells.tif') simply won't work anymore... So this comment of mine wasn't discussing naming conventions.

But 😉 I'm always happy to have a discussion about naming conventions, as @jni knows! I know that we already have cell in our data registry, but I'm not too fond of cells3D. Then, should we make sure all 3D images have a name ending with '3D'...? I prefer documenting this in the docstring.

hmaarrfk · 2020-08-31T14:39:55Z

Honestly, i don't know what to call this dataset.

Context would help. I would prefer not naming it cells3D either.

Something more descriptive of the experiment would be really nice.

alexdesiqueira · 2020-08-31T17:19:47Z

Context would help. I would prefer not naming it cells3D either.

In this case, I'd go for "if isn't broke don't fix it" 😂 cells is the best way to go (until we find a better one).

jni · 2020-09-01T02:28:53Z

Sorry, I'm not sure about the complete discussion here. Can someone summarise the proposed options? Regarding deprecating names of functions already released, I'd prefer we avoid that, even if we didn't publicise them. Someone might have done data.<tab> and used some of those functions. For stuff that hasn't been released, we can be flexible going forward.

On example is the cells-3d dataset. For that one, if we haven't actually published it in the data. namespace, I think actually we should rework it to contain both channels — there is a membranes.tif file in the tutorials repo that corresponds to the same cells. As to what to call it... I dunno, I kinda like cells3d (lowercase d), other alternatives welcome...

hmaarrfk · 2020-09-03T02:04:08Z

@mkcor would you mind helping me attribute the data correctly?

Do you have al link to the original data?

Maybe this sentence isn't correct: "Data of human cells undergoing mitosis taken from [1]_." Strictly speaking, the image wasn't taken from that paper, it was taken from CellProfiler as detailed here: https://scikit-image.org/docs/dev/auto_examples/applications/plot_human_mitosis.html#segment-human-cells-in-mitosis
(The image is part of the data used in the cited paper, but does not ship as is with the paper, even considering the supplemental material, etc.)

mkcor · 2020-09-03T08:26:50Z

skimage/data/tests/test_data.py

@@ -170,4 +173,4 @@ def test_lily_multichannel():
    """
    fetch('data/lily.tif')
    lily = data.lily()
-    assert lily.shape == (922, 922, 4)
+    assert lily.shape == (922, 922, 4)


I don't support this change 😉 (only referring to the EOF thing)

mkcor · 2020-09-03T08:49:17Z

Sorry, I'm not sure about the complete discussion here. Can someone summarise the proposed options? Regarding deprecating names of functions already released, I'd prefer we avoid that, even if we didn't publicise them. Someone might have done data.<tab> and used some of those functions. For stuff that hasn't been released, we can be flexible going forward.

@jni data.mitosis() and data.cells() have not been published yet; they are indeed missing. With this PR, @hmaarrfk is filling this gap while renaming mitosis into human_mitosis -- I don't particularly support this renaming because, then, should we rename kidney into mouse_kidney?

On example is the cells-3d dataset. For that one, if we haven't actually published it in the data. namespace, I think actually we should rework it to contain both channels — there is a membranes.tif file in the tutorials repo that corresponds to the same cells. As to what to call it... I dunno, I kinda like cells3d (lowercase d), other alternatives welcome...

Interesting, I wasn't aware of this. Ok, so let's look into this more carefully before we publish membranes and/or cells3d.

mkcor · 2020-09-03T08:54:19Z

@mkcor would you mind helping me attribute the data correctly?

Do you have a link to the original data?

Sure, it is: https://github.com/CellProfiler/examples/blob/master/ExampleHuman/images/AS_09125_050116030001_D03f00d0.tif as documented under AS_09125_050116030001_D03f00d0.tif at https://gitlab.com/scikit-image/data/#data

mkcor · 2020-09-03T15:11:54Z

mouse_kidney

s/mouse/murine/

hmaarrfk · 2020-09-03T20:29:33Z

thanks for the reference for human mitosis.

@jni do you k,now the original link for the cells3d?

emmanuelle · 2020-09-07T11:11:05Z

@jni do you k,now the original link for the cells3d?

Knowing more about the dataset might help crafting a new name.

emmanuelle · 2020-09-07T11:18:51Z

Actually I'm quite in favor of cells3d because it tells user they should not directly call plt.imshow on the image array.

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

hmaarrfk · 2020-10-29T03:08:12Z

Thanks @grlee77 i've fixed a few more thngs.

I'm a little hesitant that we are pointing to master, and not to a specific branch or to more permanent links.

That said, the sample data feels more ephemeral to me, than the library itself.

grlee77 · 2020-10-29T05:39:38Z

I see there was one other issue by @jni that remains to be addressed: #4951 (comment)

@jni, was your intention that skimage.data.cells3d should return a shape (60, 256, 256, 2) array created by stacking these two files?

Here is a plot of a single slice of the 3D datasets overlaid for context

from skimage import io
import matplotlib.pyplot as plt

m = io.imread('cells_membrane.tif')
c = io.imread('cells.tif')
fig, axes = plt.subplots(1, 3)
axes[0].imshow(c[30])
axes[0].set_title('cells.tif')
axes[1].imshow(m[30])
axes[1].set_title('cells_membrane.tif')
axes[2].imshow(c[30] + 2*m[30])
axes[2].set_title('overlaid')

jni · 2020-10-29T05:45:37Z

Yeah, that's my goal/intention. I intend to push to this branch shortly, hold please!

jni · 2020-10-29T07:55:34Z

Done! Assuming tests pass this is ok from me.

jni · 2020-10-29T10:32:32Z

🎉 🚀

hmaarrfk · 2020-10-29T11:52:40Z

Thanks everybody

jni · 2020-10-29T12:59:40Z

Thank you, @hmaarrfk! You always do the hard work and then it sits in committee for months! But we deeply appreciate it! ❤️

alexdesiqueira reviewed Aug 30, 2020

View reviewed changes

doc/examples/applications/plot_3d_image_processing.py Outdated Show resolved Hide resolved

skimage/data/__init__.py Outdated Show resolved Hide resolved

alexdesiqueira reviewed Aug 30, 2020

View reviewed changes

sciunto reviewed Aug 31, 2020

View reviewed changes

skimage/data/__init__.py Outdated Show resolved Hide resolved

sciunto reviewed Aug 31, 2020

View reviewed changes

skimage/data/__init__.py Outdated Show resolved Hide resolved

jni approved these changes Aug 31, 2020

View reviewed changes

mkcor reviewed Aug 31, 2020

View reviewed changes

skimage/data/tests/test_data.py Outdated Show resolved Hide resolved

skimage/data/__init__.py Outdated Show resolved Hide resolved

skimage/data/__init__.py Outdated Show resolved Hide resolved

skimage/data/__init__.py Outdated Show resolved Hide resolved

rfezzani mentioned this pull request Aug 31, 2020

2020's calendar of community management #4486

Closed

mkcor reviewed Sep 3, 2020

View reviewed changes

hmaarrfk and others added 16 commits October 28, 2020 23:00

Update plot_3d_image_processing.py

b17afcc

spacing

29ff9c0

move many of the details to the notes section.

c30d229

Update __init__.py

4e9ce8d

Update plot_3d_image_processing.py

b09c511

Update __init__.py

8e8ce83

Update _registry.py

b0e9432

Update __init__.py

1d1988c

Update __init__.py

27dcfd3

Update __init__.py

5da1e87

Update _registry.py

1dfc69f

Update __init__.py

61626b7

rename cell3D to cells3d

4171379

Update __init__.py

cd801e8

Update skimage/data/__init__.py

f79d77e

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

sort a few entries

84294a8

hmaarrfk force-pushed the skip_eagle branch from 3a635cc to 84294a8 Compare October 29, 2020 03:05

One last fix for cells3d

15ca126

jni added 2 commits October 29, 2020 02:53

Use multichannel cells3d image

bb365c3

Tweak cells3d docstring

2e88910

Fix cells3d test

5238dca

emmanuelle merged commit 636592a into scikit-image:master Oct 29, 2020

FirefoxMetzger mentioned this pull request Oct 30, 2020

Add rolling ball algorithm for background substraction #4851

Merged

Add missing datasets to public API #4951

Add missing datasets to public API #4951

Conversation

hmaarrfk commented Aug 30, 2020 • edited Loading

Description

Checklist

For reviewers

alexdesiqueira commented Aug 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmaarrfk commented Aug 31, 2020

pep8speaks commented Aug 31, 2020 • edited Loading

Comment last updated at 2020-10-29 08:13:43 UTC

sciunto commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sciunto commented Aug 31, 2020

jni left a comment

Choose a reason for hiding this comment

hmaarrfk commented Aug 31, 2020

mkcor commented Aug 31, 2020

hmaarrfk commented Aug 31, 2020

mkcor commented Aug 31, 2020

hmaarrfk commented Aug 31, 2020

alexdesiqueira commented Aug 31, 2020

jni commented Sep 1, 2020

hmaarrfk commented Sep 3, 2020

Choose a reason for hiding this comment

mkcor commented Sep 3, 2020

mkcor commented Sep 3, 2020

mkcor commented Sep 3, 2020

hmaarrfk commented Sep 3, 2020

emmanuelle commented Sep 7, 2020

emmanuelle commented Sep 7, 2020

hmaarrfk commented Oct 29, 2020

grlee77 commented Oct 29, 2020

jni commented Oct 29, 2020

jni commented Oct 29, 2020

jni commented Oct 29, 2020

hmaarrfk commented Oct 29, 2020

jni commented Oct 29, 2020

hmaarrfk commented Aug 30, 2020 •

edited

Loading

pep8speaks commented Aug 31, 2020 •

edited

Loading