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

Inserting center coordinates to ellipsoid #4096

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexdesiqueira
Copy link
Member

Description

Inserting center coordinates to ellipsoid.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@pep8speaks
Copy link

Hello @alexdesiqueira! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 57:49: W504 line break after binary operator
Line 58:49: W504 line break after binary operator
Line 61:49: W504 line break after binary operator
Line 62:49: W504 line break after binary operator

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

center seems not to be the correct parameter name. Isn't shift better since the ellipsoid is shifted relatively to the returned volume center?
Shouldn't the shape of the returned volume be adapted such that it contains the whole generated ellipsoid?

@@ -2,7 +2,7 @@
from scipy.special import (ellipkinc as ellip_F, ellipeinc as ellip_E)


def ellipsoid(a, b, c, spacing=(1., 1., 1.), levelset=False):
def ellipsoid(a, b, c, spacing=(1., 1., 1.), levelset=False, center=(0, 0, 0)):
Copy link
Member

Choose a reason for hiding this comment

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

center argument should be documented.

@@ -33,6 +33,8 @@ def ellipsoid(a, b, c, spacing=(1., 1., 1.), levelset=False):
if (a <= 0) or (b <= 0) or (c <= 0):
raise ValueError('Parameters a, b, and c must all be > 0')

# Receiving the center coordinates
a_center, b_center, c_center = center
Copy link
Member

Choose a reason for hiding this comment

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

Add a test for center length, otherwise the unpacking will raise an error.

@alexdesiqueira
Copy link
Member Author

alexdesiqueira commented Sep 6, 2019

@rfezzani:

center seems not to be the correct parameter name. Isn't shift better since the ellipsoid is shifted relatively to the returned volume center?

I think we should define something consistent with the other algorithms (if that's the not case, i.e., if this is a specific case for this algorithm and needs a different name, please let me know why or point me a reference).

Shouldn't the shape of the returned volume be adapted such that it contains the whole generated ellipsoid?

Maybe. If that's the case we could try to solve this two things "at once"; I'll ask for @sciunto's input in it.

@rfezzani
Copy link
Member

rfezzani commented Sep 6, 2019

Hi @alexdesiqueira, the way you defined center does not correspond to a coordinate in the returned volume.
In fact, in your implementation, when center=(0, 0, 0), the center of the ellipsoid is located at floor(a+1, b+1, c+1) , ie at the center of the returned volume. That's why I proposed to rename this parameter.

Base automatically changed from master to main February 18, 2021 18:23
@grlee77 grlee77 added the ⏩ type: Enhancement Improve existing features label Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants