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

Added Fibonacci spiral and test for it #3013

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

Atharva-Shah-2298
Copy link
Contributor

Fibonacci Sphere Method for Sphere Vertices

The fibonacci_sphere method is designed for efficiently generating points on the surface of a sphere, such as in the iterations of disperse charges. The time complexity is reduced because of the analytical solution as opposed to an optimization problem solution in disperse charges

Method Signature:

def fibonacci_sphere(n_points: int, randomize: bool) -> numpy.ndarray of shape (n_points,3)

Example Usage

from dipy.core.sphere import fibonacci_sphere,Sphere
points = fibonacci_sphere(n_points=1000, randomize=True)
sph = Sphere(xyz = points)

@pep8speaks
Copy link

pep8speaks commented Dec 14, 2023

Hello @Atharva-Shah-2298, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2023-12-29 03:46:05 UTC

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Atharva-Shah-2298,

Thank you for this! See below an initial review of your PR. Thank you for your future update

dipy/core/sphere.py Outdated Show resolved Hide resolved
dipy/core/sphere.py Outdated Show resolved Hide resolved
dipy/core/sphere.py Outdated Show resolved Hide resolved
Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Can you squash your pep8 commit ? thank you

Copy link

codecov bot commented Dec 25, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (8670d56) 81.72% compared to head (5fe8076) 82.09%.
Report is 115 commits behind head on master.

❗ Current head 5fe8076 differs from pull request most recent head eccf30b. Consider uploading reports for the commit eccf30b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3013      +/-   ##
==========================================
+ Coverage   81.72%   82.09%   +0.37%     
==========================================
  Files         147      146       -1     
  Lines       20569    20465     -104     
  Branches     3279     3260      -19     
==========================================
- Hits        16809    16800       -9     
+ Misses       2932     2855      -77     
+ Partials      828      810      -18     
Files Coverage Δ
dipy/align/streamwarp.py 78.35% <ø> (+0.93%) ⬆️
dipy/core/gradients.py 86.83% <ø> (-0.25%) ⬇️
dipy/denoise/patch2self.py 75.92% <ø> (+1.38%) ⬆️
dipy/reconst/dti.py 92.58% <100.00%> (+0.01%) ⬆️
dipy/reconst/shm.py 93.50% <ø> (ø)
dipy/stats/analysis.py 92.00% <ø> (-0.31%) ⬇️
dipy/testing/decorators.py 76.59% <100.00%> (ø)
dipy/utils/arrfuncs.py 100.00% <100.00%> (ø)
dipy/core/sphere.py 94.00% <93.33%> (-0.05%) ⬇️
dipy/reconst/sfm.py 89.85% <0.00%> (ø)
... and 1 more

... and 28 files with indirect coverage changes

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Atharva-Shah-2298,

Overall, looks good and much better! thank you for the update.

See below my last comment but it seems ready to be merged.

dipy/core/sphere.py Show resolved Hide resolved
Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. here the results:

image

with the following code:

In [1]: from dipy.core.sphere import fibonacci_sphere,Sphere

In [2]: points = fibonacci_sphere(n_points=1000, randomize=True)

In [3]: points = fibonacci_sphere(n_points=323, randomize=True)

In [4]: sph = Sphere(xyz = points)

In [5]: from dipy.viz import window, actor

In [6]: scene = window.Scene()
   ...: scene.SetBackground(1, 1, 1)

In [7]: scene.add(actor.point(sph.vertices, window.colors.red, point_radius=0.05))

In [8]: window.show(scene)

I will go ahead and merge this PR.

Thank you for this work @Atharva-Shah-2298

@skoudoro skoudoro merged commit 4c1a521 into dipy:master Dec 29, 2023
28 of 29 checks passed
@jhlegarreta jhlegarreta mentioned this pull request Mar 18, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants