Cylinder.best_fit about scikit-spatial HOT 8 CLOSED

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.

from scikit-spatial.

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!

from scikit-spatial.

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?

from scikit-spatial.

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.

from scikit-spatial.

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!

from scikit-spatial.

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

from scikit-spatial.

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

from scikit-spatial.

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.

from scikit-spatial.