ajhynes7/scikit-spatial

Cylinder.best_fit

mrrezaie opened this issue · 8 comments

mrrezaie commented

Hi,

Thanks for this interesting package. Any plan for Cylinder.best_fit?
This algorithm always works well in any data:
https://github.com/Buzzolan/Visual_Computing/blob/4a43faa1b0887a09dd8418f32982b27e821013e8/RobustFitting/LSGE/lscylinder.m

CristianoPizzamiglio commented

I am interested as well in having a best fit classmethod for the cylinder class. I studied the algorithm reported by Eberly and I hope to open a PR for that as soon as possible. Thanks for sharing the link above!

CristianoPizzamiglio commented

Hi @ajhynes7!
I hope everything's fine.

I discovered this Python repo by xingjiepan that implements David Eberly's algorithm for the best-fit cylinder (as an exercise, I have written this object-oriented implementation of xingjiepan's repo).

  1. I was thinking to re-implement all the logic directly into scikit-spatial. Does it sound good to you?
  2. The SciPy minimize function is required. Is it ok for you to have SciPy as an additional scikit-spatial's dependency?
ajhynes7 commented

Hi @CristianoPizzamiglio, sure you can go ahead and open a PR. But I'd like to keep supporting Python 3.7 for now, so I'm hoping you can use a SciPy version which is compatible with Python 3.7.

CristianoPizzamiglio commented

I'll make sure to preserve Python 3.7 compatibility.

A bunch of intermediate functions are required to run the algorithm. Where do you want to store them? Either as non-public functions within the cylinder module or within a dedicated non-public module?

Let me know if you have any other suggestion, please. Thanks!

ajhynes7 commented

Since they're not being using by any other spatial objects, I think they should be private functions in the cylinder module.

CristianoPizzamiglio commented

Sounds good, thanks! I'll open the PR soon.

CristianoPizzamiglio commented

Hi @ajhynes7 , happy 2023!

Should non-public functions have a complete docstring (parameters and returns)? The non-public functions in cylinder.py have a partial docstring (e.g. _intersect_line_with_finite_cylinder).

Thanks.

ajhynes7 commented

Hi Cristiano, you too!

For now I'll suggest to add a partial or complete docstring if you think it would help with comprehending the function. I'll have a better idea once I see these functions in the PR.

  • 👍1