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

API: disk vs circle vs circle_perimeter #4406

Open
2 of 4 tasks
sciunto opened this issue Jan 22, 2020 · 9 comments
Open
2 of 4 tasks

API: disk vs circle vs circle_perimeter #4406

sciunto opened this issue Jan 22, 2020 · 9 comments
Labels
📜 type: API Involves API change(s) 💬 Discussion
Milestone

Comments

@sciunto
Copy link
Member

sciunto commented Jan 22, 2020

Description

In morphology disk means a full circle
In Hough transform circle means the edge of a circle
In draw.circle means a full circle
In draw.circle_perimeter and draw.circle_perimeter_aa circle perimeter means the edge of a circle.
In measure circleModel, circle means edge of a circle
In measure circle_level_set circle is a full circle.

I admit this can be disturbing.

I recommend to rename

In my mind, disk = circle filled up with pixels, circle = mathematical line.

What do you think @scikit-image/core

@sciunto sciunto added this to the 1.0 milestone Jan 22, 2020
@jni
Copy link
Member

jni commented Jan 22, 2020

@sciunto I'm 👍 on this proposal! Great catch(es)!

@hmaarrfk
Copy link
Member

How will you handle the deprecation cycle for the new circle? Will you do it in a single shot or two?

@alexdesiqueira
Copy link
Member

Great catches indeed @sciunto, one more step for our consistency!

@sciunto
Copy link
Member Author

sciunto commented Jan 23, 2020

@hmaarrfk Good question. The number/type of arguments for the new disk and the new circle functions are the same. Therefore, it's hard to detect users who have an outdated code, embarrassing for a single shot.

I would suggest:

  • Step 1: warn that draw.circle is deprecated. Implement draw.disk. (For 2 versions).
  • Step 2: move the code in draw.circle_perimeter in draw.circle. Deprecated the old function. (For 2 versions).

I think this will give the best user experience.

@hmaarrfk
Copy link
Member

See PR #4462 por favor! Which addresses a few warnings introduced by the first few PRs.

@stefanv
Copy link
Member

stefanv commented Feb 17, 2020

What is the plan for implementing perimeter drawing now?

@hmaarrfk
Copy link
Member

I think it is:

  1. Keep the status quo until 2021
  2. In 2021 deprecate circle_perimeer in favor of circle

@alexdesiqueira
Copy link
Member

alexdesiqueira commented Feb 17, 2020

See PR #4462 por favor!

I guess this comment was written specifically for me :)

@stefanv
Copy link
Member

stefanv commented Feb 17, 2020

I think it is:

1. Keep the status quo until 2021

2. In 2021 deprecate circle_perimeer in favor of circle

And, there will be a keyword parameter on circle?

@grlee77 grlee77 added the 📜 type: API Involves API change(s) label Jan 25, 2021
@grlee77 grlee77 added this to In progress in skimage2 API Jan 27, 2021
@scikit-image scikit-image locked and limited conversation to collaborators Oct 18, 2021
@grlee77 grlee77 moved this from In progress to Done in skimage2 API Nov 5, 2021
@scikit-image scikit-image unlocked this conversation Mar 20, 2022
@grlee77 grlee77 modified the milestones: 1.0, skimage2 Mar 20, 2022
@grlee77 grlee77 reopened this Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 type: API Involves API change(s) 💬 Discussion
Projects
Development

No branches or pull requests

7 participants