Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Removed useless argument, added tests for wigner.racah(), Fixed error for half integers #2992

Merged
merged 22 commits into from Mar 5, 2014

Conversation

Projects
None yet
8 participants
Member

debugger22 commented Mar 3, 2014

Added tests for racah() of sympy.physics.wigner module which was not added in #2878
Removed useless prec argument from clebsch_gordan and wigner_3j.
Updated docstrings.
Fixes #2767
Fixes #2766

Contributor

akshayah3 commented Mar 3, 2014

#2985 Check this out, Prec has already been removed here.

Member

debugger22 commented Mar 3, 2014

@akshayah3 Oh! I didn't know that. Anyway that PR hasn't been merged yet and I've also done few more things beside that.

Contributor

akshayah3 commented Mar 3, 2014

Anyway I have referenced it, you two can sort it out.

Member

debugger22 commented Mar 4, 2014

@flacjacket I've no idea why the Travis build failed. I checked it my computer and it is working fine.

Member

jcrist commented Mar 4, 2014

@debugger22 if you look into the travis build, it gives 2 reasons:
1.) You forgot to remove a "prec" in clebsch_gordan on line 261.
2.) You have trailing whitespace.

Member

debugger22 commented Mar 4, 2014

Thanks! I'll fix it ASAP.
On Mar 4, 2014 10:06 PM, "Jim Crist" notifications@github.com wrote:

@debugger22 https://github.com/debugger22 if you look into the travis
build, it gives 2 reasons:
1.) You forgot to remove a "prec" in clebsch_gordan on line 261.
2.) You have trailing whitespace.

Reply to this email directly or view it on
GitHubhttps://github.com/sympy/sympy/pull/2992#issuecomment-36644451
.

skirpichev and others added some commits Mar 4, 2014

Member

debugger22 commented Mar 4, 2014

@jcrist I fixed those things what u had pointed. The build is not triggering now, is that an issue or just a propagation delay?

Contributor

akshayah3 commented Mar 4, 2014

Its a propagation delay.

Member

flacjacket commented Mar 4, 2014

Hmm, I hadn't noticed that racah didn't directly have any tests at all, except indirectly through wigner_6j.

You should also add some tests that fail before 0cae01c, but have been fixed by the int(...) changes to really say 2767 is fixed, so you need some tests that have float arguments. You should find, in fact, there are still issues. For example, you have added a test for racah(3, 3, 3, 3, 3, 3), but changing to floats, racah(3., 3., 3., 3., 3., 3.) throws an error (same is true for the existing 6j/9j/gaunt tests). It would be good to also check the result for half-integer values, if you have trouble finding ones that work, I can try to look some up, I have a book with tables of them back at my house.

Owner

certik commented Mar 4, 2014

@debugger22 I just merged #2985. Would you mind merging with master? It shouldn't introduce any major conflicts. Your PR adds some good additional stuff that we should merge.

Member

debugger22 commented Mar 4, 2014

@certik Sure! no problem, I'll rebase it first.

Member

debugger22 commented Mar 4, 2014

@certik @flacjacket Please review this. Done rebasing with master and added test for half integers for which racah() was raising error earlier

Owner

certik commented Mar 4, 2014

Funny, github is somehow confused by this PR. But checking out your branch by hand and doing:

ondrej@kittiwake:~/repos/sympy((f92fb93...))$ git diff master

produces:

diff --git a/sympy/physics/tests/test_clebsch_gordan.py b/sympy/physics/tests/test_clebsch_gordan.py
index 6fc1027..60ffa2e 100644
--- a/sympy/physics/tests/test_clebsch_gordan.py
+++ b/sympy/physics/tests/test_clebsch_gordan.py
@@ -1,5 +1,5 @@
 from sympy import S, sqrt, pi
-from sympy.physics.wigner import clebsch_gordan, wigner_9j, wigner_6j, gaunt
+from sympy.physics.wigner import clebsch_gordan, wigner_9j, wigner_6j, gaunt, racah
 from sympy.core.numbers import Rational

 # Todo: more tests should be added from:
@@ -120,3 +120,9 @@ def tn(a, b):
     assert gaunt(1, 0, 1, 1, 0, -1) == -1/(2*sqrt(pi))
     assert tn(gaunt(
         10, 10, 12, 9, 3, -12, prec=64), (-S(98)/62031) * sqrt(6279)/sqrt(pi))
+
+def test_racah():
+    assert racah(3,3,3,3,3,3) == Rational(-1,14)
+    assert racah(2,2,2,2,2,2) == Rational(-3,70)
+    assert racah(7,8,7,1,7,7, prec=4).is_Float
+    assert racah(5.5,7.5,9.5,6.5,8,9)
diff --git a/sympy/physics/wigner.py b/sympy/physics/wigner.py
index bc66615..827b51f 100644
--- a/sympy/physics/wigner.py
+++ b/sympy/physics/wigner.py
@@ -65,7 +65,7 @@ def _calc_factlist(nn):
         [1, 1, 2, 6, 24, 120, 720, 5040, 40320, 362880, 3628800]
     """
     if nn >= len(_Factlist):
-        for ii in range(len(_Factlist), nn + 1):
+        for ii in range(len(_Factlist), int(nn + 1)):
             _Factlist.append(_Factlist[ii - 1] * ii)
     return _Factlist[:int(nn) + 1]

@@ -226,7 +226,7 @@ def clebsch_gordan(j_1, j_2, j_3, m_1, m_2, m_3):

     OUTPUT:

-    Rational number times the square root of a rational number
+    Rational number times the square root of a rational number.

     EXAMPLES::

@@ -288,6 +288,7 @@ def _big_delta_coeff(aa, bb, cc, prec=None):
         sage: _big_delta_coeff(1,1,1)
         1/2*sqrt(1/6)
     """
+
     if int(aa + bb - cc) != (aa + bb - cc):
         raise ValueError("j values must be integer or half integer and fulfill the triangle relation")
     if int(aa + cc - bb) != (aa + cc - bb):
@@ -532,7 +533,7 @@ def wigner_9j(j_1, j_2, j_3, j_4, j_5, j_6, j_7, j_8, j_9, prec=None):
     imax = min(j_1 + j_9, j_2 + j_6, j_4 + j_8)

     sumres = 0
-    for kk in range(int(imin), int(imax) + 1):
+    for kk in range(imin, int(imax) + 1):
         sumres = sumres + (2 * kk + 1) * \
             racah(j_1, j_2, j_9, j_6, j_3, kk, prec) * \
             racah(j_4, j_6, j_8, j_2, j_5, kk, prec) * \

Which is 👍 from me. Let's wait for tests to finish and if all is ok, merge.

@certik certik commented on an outdated diff Mar 4, 2014

sympy/physics/tests/test_clebsch_gordan.py
@@ -120,3 +120,9 @@ def tn(a, b):
assert gaunt(1, 0, 1, 1, 0, -1) == -1/(2*sqrt(pi))
assert tn(gaunt(
10, 10, 12, 9, 3, -12, prec=64), (-S(98)/62031) * sqrt(6279)/sqrt(pi))
+
+def test_racah():
+ assert racah(3,3,3,3,3,3) == Rational(-1,14)
+ assert racah(2,2,2,2,2,2) == Rational(-3,70)
+ assert racah(7,8,7,1,7,7, prec=4).is_Float
+ assert racah(5.5,7.5,9.5,6.5,8,9)
@certik

certik Mar 4, 2014

Owner

In here, in addition shouldn't we test the actual result?

In [1]: from sympy.physics.wigner import racah

In [2]: racah(5.5,7.5,9.5,6.5,8,9)
Out[2]: 
       _____ 
-719⋅╲╱ 598  
─────────────
   1158924   

In [3]: racah(5.5,7.5,9.5,6.5,8,9, prec=4)
Out[3]: -0.01517

i.e. test within some eps etc.

Member

debugger22 commented Mar 4, 2014

I am new to sympy, I don't know how to represent those numbers. I tried doing what you just said like above two cases.

Owner

certik commented Mar 4, 2014

Ah I see. The number

       _____ 
-719⋅╲╱ 598  
─────────────
   1158924   

is represented like this:

In [4]: -719*sqrt(598)/1158924
Out[4]: 
       _____ 
-719⋅╲╱ 598  
─────────────
   1158924   

and as to the float, just do something like:

assert abs(racah(5.5,7.5,9.5,6.5,8,9, prec=4) - (-0.01517)) < 1e-4
Member

debugger22 commented Mar 4, 2014

Thanks! I'm on it.

Member

debugger22 commented Mar 4, 2014

Done! 😄

Owner

certik commented Mar 4, 2014

Yep, it works now. Btw, the numerical value is obtained as:

In [18]: (-719*sqrt(598)/1158924).n()
Out[18]: -0.0151713604143125

But that doesn't matter. 👍 to merge if tests pass.

Member

debugger22 commented Mar 5, 2014

Build passed at last :)

Owner

certik commented Mar 5, 2014

Cool, merging.

@certik certik added a commit that referenced this pull request Mar 5, 2014

@certik certik Merge pull request #2992 from debugger22/wignerfix
Removed useless argument, added tests for wigner.racah(), Fixed error for half integers
870f0b0

@certik certik merged commit 870f0b0 into sympy:master Mar 5, 2014

1 check passed

default The Travis CI build passed
Details

@debugger22 debugger22 deleted the debugger22:wignerfix branch Mar 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment