API updates before 0.16 #4187
API updates before 0.16 #4187
Conversation
|
Hello @jni! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-10-04 00:56:05 UTC |
I'd maybe prefer The PR looks good so far 👍 . |
| @@ -19,7 +19,7 @@ def _match_cumulative_cdf(source, template): | |||
| return interp_a_values[src_unique_indices].reshape(source.shape) | |||
|
|
|||
|
|
|||
| def match_histograms(image, reference, multichannel=False): | |||
| def match_histograms(image, reference, *, multichannel=False): | |||
There was a problem hiding this comment.
This was released with 0.15, so it is an API change https://scikit-image.org/docs/stable/api/skimage.transform.html#match-histograms
There was a problem hiding this comment.
No, skimage.exposure.match_histograms is a brand new function! =)
| def structural_similarity(im1, im2, win_size=None, gradient=False, | ||
| data_range=None, multichannel=False, | ||
| gaussian_weights=False, full=False, **kwargs): | ||
| def structural_similarity(im1, im2, |
There was a problem hiding this comment.
I thought we didn't like im or img in signatures and prefered image. Should we fix here?
There was a problem hiding this comment.
Correct, just waiting for the discussion above to settle...
There was a problem hiding this comment.
Noticing these im1, im2 as I'm reviewing #6612 and... it hurts my eyes! 😢
I'm adding this item to the list of Pending API changes for 1.0.
| `diff` computes the absolute difference between the two images. | ||
| `blend` computes the mean value. | ||
| `checkerboard` makes tiles of dimension `n_tiles` that display | ||
| ``'diff'`` computes the absolute difference between the two images. |
There was a problem hiding this comment.
We must keep single backquotes now... (I'm responsible for the mess...)
There was a problem hiding this comment.
@sciunto I think actually in this case this is better, because it's a literal string, not an argument name.
There was a problem hiding this comment.
You're right, the rendering will be better.
|
I have a small suggestion concerning the API of optical flow algorithms: since only two values are allowed for the |
|
Otherwise, both names are fine to me ( |
Hmm, I think I slightly prefer to keep the dtype argument because it is more flexible. The fact that only two values are allowed is not because it is a "true" flag that modifies the logic of the algorithm (please correct me if I'm wrong) but because those are the limits of the technology used. If in the future another dtype became available we'd have to change the API again. But maybe I'm overthinking it. ;) Why do you think a flag would be better? |
|
Optical flow will always be estimated as float. Using single or double precision is a matter of treadoff between precision and memory use/computation speed. |
|
@rfezzani Thanks for the insights. I don't see the check and raise as a disadvantage. Though I see your point that |
|
Hi everyone, After discussing with Stéfan in the community meeting this week, we settled on the following:
@NelleV has volunteered to be release manager and we've kept her waiting for a while, so I would like everyone who wants to have a say here to have one last review and then (hopefully) click approve. =) If no one has strong objections in the next ~24h, we will start pushing out 0.16 this time tomorrow. Have a nice day! =) |
| def _tvl1(image0, image1, flow0, attachment, tightness, nwarp, niter, | ||
| tol, prefilter): | ||
| def _tvl1(reference_image, moving_image, flow0, attachment, tightness, nwarp, | ||
| niter, tol, prefilter): |
| dtype='float32'): | ||
| def optical_flow_tvl1(reference_image, moving_image, | ||
| *, | ||
| attachment=15, tightness=0.3, nwarp=5, niter=10, |
There was a problem hiding this comment.
The same here: niter -> num_iter
How about |
I like reference... |
I agree with that. Let's keep |
There was a problem hiding this comment.
I am +1 on the naming conventions chosen here. I think I would have used image1, image2 instead of (image0, image1), but it is not a strong preference.
Once the test failures are resolved and the last bits completed, I approve merging this.
I commented on one of the bugs. I saw another one in the logs related to variation_of_information:
def test_vi():
im_true = np.array([1, 2, 3, 4])
im_test = np.array([1, 1, 8, 8])
> assert_equal(np.sum(variation_of_information(im_true, im_test)), 1)
c:\hostedtoolcache\windows\python\3.7.4\x64\lib\site-packages\skimage\metrics\tests\test_segmentation_metrics.py:29:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
image0 = array([1, 2, 3, 4]), image1 = array([1, 1, 8, 8])
def variation_of_information(image0=None, image1=None, *, table=None,
ignore_labels=()):
"""Return symmetric conditional entropies associated with the VI. [1]_
The variation of information is defined as VI(X,Y) = H(X|Y) + H(Y|X).
If X is the ground-truth segmentation, then H(X|Y) can be interpreted
as the amount of under-segmentation and H(X|Y) as the amount
of over-segmentation. In other words, a perfect over-segmentation
will have H(X|Y)=0 and a perfect under-segmentation will have H(Y|X)=0.
Parameters
----------
image0, image1 : ndarray of int
Label images / segmentations, must have same shape.
table : scipy.sparse array in csr format, optional
A contingency table built with skimage.evaluate.contingency_table.
If None, it will be computed with skimage.evaluate.contingency_table.
If given, the entropies will be computed from this table and any images
will be ignored.
ignore_labels : sequence of int, optional
Labels to ignore. Any part of the true image labeled with any of these
values will not be counted in the score.
Returns
-------
vi : ndarray of float, shape (2,)
The conditional entropies of image1|image0 and image0|image1.
References
----------
.. [1] Marina Meil\u0103 (2007), Comparing clusterings\u2014an information based
distance, Journal of Multivariate Analysis, Volume 98, Issue 5,
Pages 873-895, ISSN 0047-259X, :DOI:`10.1016/j.jmva.2006.11.013`.
"""
h0g1, h1g0 = _vi_tables(image0, image1, table=table,
> ignore_labels=ignore_labels, normalize=True)
E TypeError: _vi_tables() got an unexpected keyword argument 'normalize'
|
+1; @jni one observation: in comparison functions, we have |
|
@stefanv huh? Optical flow uses |
|
Ditto for |
|
Ah. Sure but that's not registration. =) I think in that case it makes more sense, because you are modifying the input image to match a reference. The output is a modified version of the first input. In registration, the output is something that relates the first input to the second. So I don't think it's a critical inconsistency. But anyway, thanks for pointing it out! It's something to consider for 1.0 I think. =) |
|
👍 I'd merge, but you said you had one more thing to change? |
|
Nope, all done. =P |
Description
Using @Carreau's great tool frappuccino, I went through all the new APIs we are releasing in 0.16 and checked that they were where we want them to be. Mostly it was a matter of making some arguments keyword-only.
The only thing missing right now is in the new
metricsandregistrationmodules, and also in the newutil.compare_imagesfunction, some functions useim1, im2, others useim_true, im_test, whilecompare_imagesusesimage1, image2and the new optical flow usesimage0, image1. Needless to say this is a mess and we should aim to rectify it. Suggestions welcome! My preference isreference_image, target_image.Checklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.@meeseeksdev backport to v0.14.x