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

Fix zernike_eval notebook and Use integer division instead of gammaln() #1037

Merged
merged 23 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c55fc45
fix typo in the zernike radial formula
YigitElma May 26, 2024
6b6c937
fix the exact calculation to use scipy factorial which is more accurate
YigitElma May 26, 2024
92726e4
fix exact factorials
YigitElma May 26, 2024
c66da6b
update notebook
YigitElma May 27, 2024
4d9180b
use decimal package for increased accuracy in division
YigitElma May 27, 2024
606a930
math and scipy.special factorial are the same if you set exact=True f…
YigitElma May 27, 2024
0cbc083
fix exact issue
YigitElma May 27, 2024
d2aeb79
fix factorial accuracy in profile calculation
YigitElma May 27, 2024
f8f1887
undo decimal thing
YigitElma May 29, 2024
f95104d
fix <>:32: SyntaxWarning: invalid escape sequence '\m' warning issue
YigitElma May 29, 2024
ec5b9a5
fix <>:32: SyntaxWarning: invalid escape sequence '\m' warning issue
YigitElma May 29, 2024
7752a5f
add comment for integer division
YigitElma May 31, 2024
347807e
Merge branch 'master' into yge/hotfix
YigitElma Jun 1, 2024
c660cca
make notebook more compact
YigitElma Jun 10, 2024
8a1d505
add better method description
YigitElma Jun 10, 2024
3eb59da
Merge branch 'yge/hotfix' of github.com:PlasmaControl/DESC into yge/h…
YigitElma Jun 10, 2024
79388b7
update notebook typo
YigitElma Jun 11, 2024
f5766d0
update gamma part in jacobi
YigitElma Jun 12, 2024
1f1e3a9
update master_compute_data.pkl
YigitElma Jun 12, 2024
28d4785
Merge branch 'master' into yge/hotfix
YigitElma Jun 12, 2024
e6073af
Merge branch 'master' into yge/hotfix
YigitElma Jun 14, 2024
bb8febd
Merge branch 'master' into yge/hotfix
YigitElma Jun 18, 2024
7b164ef
Merge branch 'master' into yge/hotfix
YigitElma Jun 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 29 additions & 14 deletions desc/basis.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import mpmath
import numpy as np

from desc.backend import custom_jvp, fori_loop, gammaln, jit, jnp, sign
from desc.backend import custom_jvp, fori_loop, jit, jnp, sign
from desc.io import IOAble
from desc.utils import check_nonnegint, check_posint, flatten_list

Expand Down Expand Up @@ -1389,6 +1389,16 @@ def body(k, y):
def zernike_radial_coeffs(l, m, exact=True):
"""Polynomial coefficients for radial part of zernike basis.

The for loop ranges from m to l+1 in steps of 2, as opposed to the
formula in the zernike_eval notebook. This is to make the coeffs array in
ascending powers of r, which is more natural for polynomial evaluation.
So, one should substitute s=(l-k)/s in the formula in the notebook to get
the coding implementation below.

(-1)^((l-k)/2) * ((l+k)/2)!
R_l^m(r) = sum_{k=m}^l -------------------------------------
((l-k)/2)! * ((k+m)/2)! * ((k-m)/2)!

Parameters
----------
l : ndarray of int, shape(K,)
Expand Down Expand Up @@ -1424,14 +1434,15 @@ def zernike_radial_coeffs(l, m, exact=True):
ll = lms[ii, 0]
mm = lms[ii, 1]
for s in range(mm, ll + 1, 2):
coeffs[ii, s] = (
(-1) ** ((ll - s) // 2)
* factorial((ll + s) // 2)
// (
factorial((ll - s) // 2)
* factorial((s + mm) // 2)
* factorial((s - mm) // 2)
)
# Zernike polynomials can also be written in the form of [1] which
# states that the coefficients are given by the binomial coefficients
# hence they are all integers. So, we can use exact arithmetic with integer
# division instead of floating point division.
# [1]https://en.wikipedia.org/wiki/Zernike_polynomials#Other_representations
coeffs[ii, s] = ((-1) ** ((ll - s) // 2) * factorial((ll + s) // 2)) // (
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's no difference here right? Just the extra comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there is no change. Additional lines confused git, I guess.

factorial((ll - s) // 2)
* factorial((s + mm) // 2)
* factorial((s - mm) // 2)
)
c = np.fliplr(np.where(lm_even, coeffs, 0))
if not exact:
Expand Down Expand Up @@ -1762,12 +1773,16 @@ def _jacobi_body_fun(kk, d_p_a_b_x):
n, alpha, beta, x = map(jnp.asarray, (n, alpha, beta, x))

# coefficient for derivative
c = (
gammaln(alpha + beta + n + 1 + dx)
- dx * jnp.log(2)
- gammaln(alpha + beta + n + 1)
coeffs = jnp.array(
[
1,
(alpha + n + 1) / 2,
(alpha + n + 2) * (alpha + n + 1) / 4,
(alpha + n + 3) * (alpha + n + 2) * (alpha + n + 1) / 8,
(alpha + n + 4) * (alpha + n + 3) * (alpha + n + 2) * (alpha + n + 1) / 16,
]
)
c = jnp.exp(c)
c = coeffs[dx]
# taking derivative is same as coeff*jacobi but for shifted n,a,b
n -= dx
alpha += dx
Expand Down
4 changes: 2 additions & 2 deletions desc/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,8 @@
def multinomial_coefficients(m, n):
"""Number of ways to place n objects into m bins."""
k = combination_permutation(m, n)
num = factorial(n)
den = factorial(k).prod(axis=-1)
num = factorial(n, exact=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does exact=True do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, we are using scipy s factorial function which is by default an approximation with gamma function. To be more accurate, we should use integer operations instead. Exact=true converts it to integer operation. https://docs.scipy.org/doc/scipy/reference/generated/scipy.special.factorial.html

den = factorial(k, exact=True).prod(axis=-1)

Check warning on line 461 in desc/utils.py

View check run for this annotation

Codecov / codecov/patch

desc/utils.py#L460-L461

Added lines #L460 - L461 were not covered by tests
return num / den


Expand Down
592 changes: 153 additions & 439 deletions docs/notebooks/zernike_eval.ipynb
YigitElma marked this conversation as resolved.
Show resolved Hide resolved

Large diffs are not rendered by default.

Binary file modified tests/inputs/master_compute_data.pkl
Binary file not shown.
Loading