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

ENH: scipy.special, Implemented Ellipsoidal harmonic function: the first kind, the second kind and normalization constant #3811

Closed
wants to merge 4 commits into from

Conversation

jennystone
Copy link
Contributor

This is an attempt to implement ellipsoidal harmonic functionality.
The functions implemented are
ellip_harm, ellip_harm_2, ellip_normal
The sources referred to are:
http://www.geology.osu.edu/~jekeli.1/OSUReports/reports/report_499.pdf
http://arxiv.org/abs/1204.0267

… first kind, the second kind and normalization constant
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling e294b1a on jennystone:ellip_3 into e758c48 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling e294b1a on jennystone:ellip_3 into e758c48 on scipy:master.

@rgommers
Copy link
Member

@jennystone for next time: it would be better to have more smaller commits. Looks like you squashed everything together now in one large commit.

>>> from scipy.special import ellip_harm_2
>>> w = ellip_normal(5,8,3,7)
>>> w
>>> 1723.38796997
Copy link
Member

Choose a reason for hiding this comment

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

Output doesn't have >>>, should be only the number on this line.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 25d7787 on jennystone:ellip_3 into e758c48 on scipy:master.

"""
return _ellip_harm(h2, k2, n, p, s, signm, signn)

def ellip_harm_2(h2, k2, n, p, s):
Copy link
Member

Choose a reason for hiding this comment

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

The definition should be changed so that it can evaluate several values at once:

# np.vectorize does not work on Cython functions on Numpy < 1.8, so a wrapper is needed
def _ellip_harm_2_vec(h2, k2, n, p, s):
    return _ellipsoid(h2, k2, n, p, s)
_ellip_harm_2_vec = np.vectorize(_ellip_harm_2_vec, otypes='d')

def ellip_harm_2(h2, k2, n, p, s):
   ...
   with _ellip_lock:
       return _ellip_harm_2_vec(h2, k2, n, p, s)

The issue is that _ellipsoid is not an ufunc, and evaluates only one value at a time, so it needs to be wrapped.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 689b5fe on jennystone:ellip_3 into 686537d on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling bcdc8b5 on jennystone:ellip_3 into 686537d on scipy:master.

return 1/res
assert_almost_equal(recursion(100, 5, 2, 41, 5, 2, 15, 25), potential(100, 5, 2, 41, 5, 2, 15, 25))
assert_almost_equal(recursion(120, sqrt(19), 2, 41, sqrt(17), 2, 15, 25), potential(120, sqrt(19), 2, 41, sqrt(17), 2, 15, 25))
def test_ellip_norm():
Copy link
Member

Choose a reason for hiding this comment

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

Style: empty line(s) before def ...:

@pv
Copy link
Member

pv commented Jul 29, 2014

I think we are getting quite close to finishing this up. The potential test proves that the implemented functions work as they should (at least within sensible parameter ranges).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling d42868c on jennystone:ellip_3 into c2375ee on scipy:master.

…es, Implementation of test with Electric Potential values, Improved implementation of ellipsoidal harmonics
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 676782d on jennystone:ellip_3 into c2375ee on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 676782d on jennystone:ellip_3 into c2375ee on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 676782d on jennystone:ellip_3 into c2375ee on scipy:master.

_F_integrand4_t = ctypes.CFUNCTYPE(ctypes.c_double, ctypes.c_double)
_F_integrand4_ctypes = ctypes.cast(<size_t>&_F_integrand4, _F_integrand4_t)

cdef double * calc_coefficients(double h2, double k2, int n, int p):
Copy link
Member

Choose a reason for hiding this comment

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

This routine is not necessary. You also don't need to store bufferp as a global variable as it can be a local variable in each of the routines.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling c280242 on jennystone:ellip_3 into c2375ee on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling a68e518 on jennystone:ellip_3 into c2375ee on scipy:master.

signn = signn/fabs(signn)

if n < 0:
sf_error.error("ellip_harm", sf_error.ARG, "invalid value for p")
Copy link
Member

Choose a reason for hiding this comment

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

"invalid value for n"

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 15c5cfb on jennystone:ellip_3 into 13a73f4 on scipy:master.

@pv pv removed the PR label Aug 13, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 48d51ac on jennystone:ellip_3 into 276b3bc on scipy:master.

@pv pv mentioned this pull request Nov 22, 2014
@pv
Copy link
Member

pv commented Nov 22, 2014

Merged in gh-4182. Thanks!

@pv pv closed this Nov 22, 2014
@pv pv added this to the 0.15.0 milestone Nov 22, 2014
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

4 participants