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

Restructuring of API #2538

Open
6 of 10 tasks
adius opened this issue Feb 24, 2017 · 9 comments
Open
6 of 10 tasks

Restructuring of API #2538

adius opened this issue Feb 24, 2017 · 9 comments

Comments

@adius
Copy link
Task lists! Give feedback

@adius adius commented Feb 24, 2017

While I was building http://feram.co/scikit-image-cheatsheet I noticed several inconsistencies in the scikit-image API. I'll just layout how I would restructure the API. Sorry for any duplicates with other issues and this should probably be split into several issues later 😅😇.

Inconsistent argument names

image vs. img vs. im vs. …

Done in #2617

I think it should be image everywhere.

Outliers:

  • transform.hough_ellipse
  • transform.hough_line
  • transform.hough_line_peaks (@jni says: possibly won't fix — different type of input?)
  • transform.integral_image
  • draw.set_color
  • feature.daisy
  • feature.draw_multiblock_lbp
  • feature.multiblock_lbp

I probably missed a few ...

label vs label_image

I'm in favor of label_image, but I figured pythonistas like unreadable concise shortcuts. I guess label is good as well …
Or maybe it should also be just image?

Outliers:

  • segmentation.find_boundaries
  • segmentation.mark_boundaries

row vs. rvs.rowsvs.rr

Definitely row, rows, col & cols (Actually I'd prefer column and columns, but I guess that's too long for python =D)

There are definitely more inconsistent argument names I just can't remember them right now ...

Create new shapes module

There are so many shapes in the morphology module. I think they deserve their own module. I found kind of surprising to found the shape methods intermingled with the morphology operations.

This means we would have shape.{cube,diamond,disk,octagon,octahedron,rectangle,square,star}

Create new binarize module

Sciunto: See #2121 and #2490

While it makes sense to be able to retrieve the threshold generated by a certain thresholding algorithm, I think most people will simply want to use it to binarize their images. That's why a propose a module corresponding to the thresholds.

  • binarize.isodata(image)
  • binarize.li(image)
  • binarize.local(image)
  • binarize.niblack(image)
  • binarize.otsu(image)
  • binarize.sauvola(image)
  • binarize.triangle(image)
  • binarize.yen(image)
  • binarize.all(image)

They expect an (grayscale) image and return the binarized image. Simple as that.

Also the thresholds should probably be moved to their own module as well.
I mean you already prefix all of them with threshold_. Why not use threshold. and make it a proper module? 😄
Btw: The API of the thresholds is also kind of chaotic. Some return images, some simple values. There should be at least an argument flag for each one to always return an image.

[WIP] - I need to get back to work ... will fill out the rest later =P

@stefanv
Copy link
Member

@stefanv stefanv commented Feb 24, 2017

I love your cheat sheet! And thank you for the API review; that's very helpful.

image vs. img vs. im vs. …

This should be image.

label vs label_image

This should be label_image.

row vs. rvs.rowsvs.rr

My feeling here is that we may want to go with rr, cc. "cols, rows" can be confusing, because it suggests "nr_of_cols, nr_of_rows", whereas we want "row_coords, column_coords".

/cc @jni @ahojnnes

Create new shapes module

I'm happy to do this via the standard deprecation path (2 releases).

Create new binarize module

I think the threshold applications should rather be updated to return binary_image, threshold_value. IIRC, there's a discussion about this elsewhere.

Would you be so kind as to file separate issues for all of these?

@tonysyu
Copy link
Member

@tonysyu tonysyu commented Mar 1, 2017

I love your cheat sheet!

Agreed! That's quite handy

row vs. r vs. rows vs. rr

Personally, I prefer i_row or i_rows (depending on whether it's an int or array of ints), and i_col or i_cols. It instantly distinguishes it from n_rows/n_cols and maps to normal indexing conventions.


Ok, I'll retreat back to my dark corner now. (Sadly, I don't do any image processing anymore, hence my long absence.)

@sciunto
Copy link
Member

@sciunto sciunto commented Apr 19, 2017

I'm OK to change img to image in signatures, but I don't think it's necessarily a good idea for variable names in examples. In doctests for example, it's convenient to have a short variable name.

@hmaarrfk
Copy link
Member

@hmaarrfk hmaarrfk commented Sep 12, 2019

I think we should use image. The img shorthand should be dropped in favor of encouraging readability and breaking the 79 character limit.

@jni
Copy link
Member

@jni jni commented Sep 13, 2019

I agree with @hmaarrfk, image everywhere. It's much nicer to read. (Back in the day I favoured im but I'm very happy I was overruled!)

@sciunto
Copy link
Member

@sciunto sciunto commented Sep 13, 2019

I remember we had this discussion somewhere, I can'f find it anymore, and I said I agree for image in the API, but I still like img in examples for the shortness.

@stefanv
Copy link
Member

@stefanv stefanv commented Sep 13, 2019

🙄

#2538 (comment)

@sciunto
Copy link
Member

@sciunto sciunto commented Sep 13, 2019

I thought I was on the other issue :D Hard to get the head up from email notifications (and my search was yesterday).

@sciunto sciunto added this to the 1.0 milestone Sep 13, 2019
@alexdesiqueira
Copy link
Member

@alexdesiqueira alexdesiqueira commented Sep 13, 2019

Hi all,
personally, as I discussed a while ago with @stefanv (and maybe I said something around to @jni as well), we need to think on an API that could embrace nD in the future.
One example would be the draw functions: they should receive center=() instead of rows, cols or rows, cols, channel.
On shorter vs longer names, I'm explicit all over it. :) image, binary_image, label_image, rows, cols, ... 😅

@grlee77 grlee77 added the api label Jan 25, 2021
@grlee77 grlee77 added this to In progress in scikit-image 1.0 API Jan 27, 2021
@grlee77 grlee77 changed the title Resctructuring of API Restructuring of API Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Local Binary Pattern
  
Awaiting triage
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants