Repository: sympy/sympy
Pull request: Patched Wigner3j not working with float that were half integers
This pull request addresses an issue where the Wigner3j function would raise a ValueError when encountering half-integer inputs recognized as floats. The problem stemmed from incomplete validation checks, which failed to properly handle such inputs.

#### References to other Issues or PRs
Fixed #26219


#### Brief description of what is fixed or changed
To resolve this issue, I implemented enhanced validation checks within the function. Now, all inputs are meticulously validated to ensure they are either integers or half-integers. This modification ensures robustness and prevents the function from failing due to invalid input types.


<!-- BEGIN RELEASE NOTES -->

NO ENTRY


<!-- END RELEASE NOTES -->

  




Problem statement:
Wigner3j has problems with Half Integers 
I was trying to find the result of this operation and am getting this error for half integers
```
>>> w3j = Wigner3j(0,0,1/2,1/2,1/2,-1/2)    
>>> w3j.doit()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "D:\Orgs\sympy\sympy\physics\quantum\cg.py", line 158, in doit 
    return wigner_3j(self.j1, self.j2, self.j3, self.m1, self.m2, self.m3)
  File "D:\Orgs\sympy\sympy\physics\wigner.py", line 199, in wigner_3j
    raise ValueError("j values must be integer or half integer")      
ValueError: j values must be integer or half integer
>>> w3j = Wigner3j(0.5,0.5,0.5,0.5,0.5,0.5)
>>> w3j.doit()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "D:\Orgs\sympy\sympy\physics\quantum\cg.py", line 158, in doit 
    return wigner_3j(self.j1, self.j2, self.j3, self.m1, self.m2, self.m3)
  File "D:\Orgs\sympy\sympy\physics\wigner.py", line 199, in wigner_3j    raise ValueError("j values must be integer or half integer")      
ValueError: j values must be integer or half integer
```
It is working fine for integer values but gives an error of values with half integers .

Hints from issue discussion:
What happens if rational numbers are used instead of floats?
```
import sympy
from sympy import Rational
import sympy.physics.wigner as w

a = Rational(1,2)
b=w.Wigner3j(a,a,a,a,a,a)
b.doit()

0
```
I think Rationals are working fine in my case , although I dont think there should be any difference in handling sympy rationals and floating point numbers here .
This issue seems to be specific to the inbuilt sympy shell that we run in the local repository . The examples used by me initially are working fine on colab . 

For Google Colab
```
import sympy
from sympy import Rational
import sympy.physics.wigner as w
a = 0.5
b=w.Wigner3j(a,a,a,a,a,a)
b.doit()
0
```


For bin/isympy
```
>>> import sympy.physics.wigner as w
>>> a = Rational(1,2)
>>> b = 1/2
>>> c = w.Wigner3j(a,a,a,a,a,a)
>>> c.doit()
0
>>> d= w.Wigner3j(b,b,b,b,b,b)  
>>> d.doit()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "D:\Orgs\sympy\sympy\physics\wigner.py", line 916, in doit     
    return wigner_3j(*self.args)
  File "D:\Orgs\sympy\sympy\physics\wigner.py", line 199, in wigner_3j
    raise ValueError("j values must be integer or half integer")      
ValueError: j values must be integer or half integer
```

Rationals and floats are inherently different in SymPy. For example, consider `(2**54-1)/2**55` vs `Rational(2**54-1, 2**55)`.
With SymPy 1.12 we have:
```python
In [1]: from sympy.physics.wigner import Wigner3j

In [2]: Wigner3j(0,0,1/2,1/2,1/2,-1/2).doit()
Out[2]: 0
```
Probably the difference is that now floats and rationals don't compare equal any more.
@oscarbenjamin so what do you suggest?

@T-vaccari The Value error is being handled inside the `wigner_3j` function so I dont't think we  need to handle it in the `doit` method . 
"We need to ensure that when inserting half-integer values, we use the `Rational` function, as opposed to Python's standard floating-point division (b = 1/2). This ensures accurate representation and proper handling of half-integer values in SymPy functions like Wigner3j, avoiding potential issues related to floating-point arithmetic
```
>>> from sympy.physics.quantum.cg import Wigner3j
>>> a = Rational(1,2)
>>> b = 0.5
>>> int(a*2)==a*2
True
>>> int(b*2)==b*2 
True
>>> w3j = Wigner3j(a,a,a,a,a,a)
>>> w3j

⎛1/2  1/2  1/2⎞
⎜             ⎟
⎝1/2  1/2  1/2⎠
>>> w3j.doit()
0
>>> w3j = Wigner3j(b,b,b,b,b,b) 
>>> w3j

⎛0.5  0.5  0.5⎞
⎜             ⎟
⎝0.5  0.5  0.5⎠
>>> w3j.doit()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "D:\Orgs\sympy\sympy\physics\quantum\cg.py", line 158, in doit 
    return wigner_3j(self.j1, self.j2, self.j3, self.m1, self.m2, self.m3)
  File "D:\Orgs\sympy\sympy\physics\wigner.py", line 199, in wigner_3j
    raise ValueError("j values must be integer or half integer")      
ValueError: j values must be integer or half integer
```

This behaviour is very ambiguous given the code handling this 
```
if int(j_1 * 2) != j_1 * 2 or int(j_2 * 2) != j_2 * 2 or \
            int(j_3 * 2) != j_3 * 2:
        raise ValueError("j values must be integer or half integer")
```
Is it happening cause of changes in data type . I printed the types of j values and got the following 

```
<class 'sympy.core.numbers.Half'> <class 'sympy.core.numbers.Half'> <class 'sympy.core.numbers.Half'>
<class 'sympy.core.numbers.Float'> <class 'sympy.core.numbers.Float'> <class 'sympy.core.numbers.Float'>
```

I suggest using some method to convert them all to a uniform type but as mentioned in several other places would be a problem with the ducktyping . I'd say we should find out where the parameters are being converted to the the above types . 

@oscarbenjamin Can you please explain why we are using the sympy number types and why some number can be compared to their python counterparts but others can't . Is it a flaw or something intentionally designed .
The solution to the problem is to ensure that all input values provided to the Wigner3j function are represented using SymPy number types, such as Rational. Maybe we can work on this if we are allowed to.
What do you think @oscarbenjamin, @shishir-11 ?

Commit message:                                                            Wigner3j : Fix inner function and add test case

This commit addresses an issue with the inner function in the
wigner_3j calculation method in the sympy.physics.wigner module.
The inner function has been fixed to ensure accurate calculation
of the Wigner 3j symbol.

Additionally, a new test case has been added to the
sympy.physics.quantum.tests.test_cg module to validate the
correctness of the Wigner 3j calculation. This test case covers
a scenario involving specific input values to thoroughly
test the functionality.
Example : Right now you can provide imput as follow without getting valuerror : Wigner3j(1/2,1/2,1/2,1/2,1/2,1/2)
References: #12345

Patch to review:
diff --git a/sympy/physics/quantum/tests/test_cg.py b/sympy/physics/quantum/tests/test_cg.py
index 7f04d386d713..e5ff16265f8e 100644
--- a/sympy/physics/quantum/tests/test_cg.py
+++ b/sympy/physics/quantum/tests/test_cg.py
@@ -171,6 +171,8 @@ def test_cg_simp_sum():
 
 def test_doit():
     assert Wigner3j(S.Half, Rational(-1, 2), S.Half, S.Half, 0, 0).doit() == -sqrt(2)/2
+    assert Wigner3j(1/2,1/2,1/2,1/2,1/2,1/2).doit() == 0
+    assert Wigner3j(9/2,9/2,9/2,9/2,9/2,9/2).doit() ==  0
     assert Wigner6j(1, 2, 3, 2, 1, 2).doit() == sqrt(21)/105
     assert Wigner6j(3, 1, 2, 2, 2, 1).doit() == sqrt(21) / 105
     assert Wigner9j(
diff --git a/sympy/physics/wigner.py b/sympy/physics/wigner.py
index e49360df15f7..3a6f4f5bb4e1 100644
--- a/sympy/physics/wigner.py
+++ b/sympy/physics/wigner.py
@@ -107,7 +107,7 @@ def _calc_factlist(nn):
             _Factlist.append(_Factlist[ii - 1] * ii)
     return _Factlist[:int(nn) + 1]
 
-
+from fractions import Fraction
 def wigner_3j(j_1, j_2, j_3, m_1, m_2, m_3):
     r"""
     Calculate the Wigner 3j symbol `\operatorname{Wigner3j}(j_1,j_2,j_3,m_1,m_2,m_3)`.
@@ -194,12 +194,34 @@ def wigner_3j(j_1, j_2, j_3, m_1, m_2, m_3):
 
     - Jens Rasch (2009-03-24): initial version
     """
-    if int(j_1 * 2) != j_1 * 2 or int(j_2 * 2) != j_2 * 2 or \
-            int(j_3 * 2) != j_3 * 2:
+
+    def convert_float_to_rational_if_half_integer(value):
+        if isinstance(value, float):
+            fraction_value = Fraction(value).limit_denominator()
+            if fraction_value.denominator == 2:
+                return fraction_value
+        return value
+
+    # Example list of values
+    values = [j_1, j_2, j_3, m_1, m_2, m_3]
+
+    # Apply the conversion function to each value using a list comprehension
+    converted_values = [convert_float_to_rational_if_half_integer(value) for value in values]
+    j_1, j_2, j_3, m_1, m_2, m_3 = converted_values
+
+    # Define a tolerance for comparison
+    tolerance = 1e-10
+
+    # Check if each value is either an integer or a half-integer
+    if abs(j_1 * 2 - int(j_1 * 2)) > tolerance or \
+            abs(j_2 * 2 - int(j_2 * 2)) > tolerance or \
+            abs(j_3 * 2 - int(j_3 * 2)) > tolerance:
         raise ValueError("j values must be integer or half integer")
-    if int(m_1 * 2) != m_1 * 2 or int(m_2 * 2) != m_2 * 2 or \
-            int(m_3 * 2) != m_3 * 2:
+    if abs(m_1 * 2 - int(m_1 * 2)) > tolerance or \
+            abs(m_2 * 2 - int(m_2 * 2)) > tolerance or \
+            abs(m_3 * 2 - int(m_3 * 2)) > tolerance:
         raise ValueError("m values must be integer or half integer")
+
     if m_1 + m_2 + m_3 != 0:
         return S.Zero
     prefid = Integer((-1) ** int(j_1 - j_2 - m_3))


Code review comments:
diff_hunk: @@ -107,7 +107,7 @@ def _calc_factlist(nn):
             _Factlist.append(_Factlist[ii - 1] * ii)
     return _Factlist[:int(nn) + 1]
 
-
+from fractions import Fraction
path: sympy/physics/wigner.py
text: @user1:
Why are you importung here?

@author:
WIthout it was not working


@user1:
Ok but why are you importing here and not at the top of the script? Is there a reason?

@author:
No there's no reason , I can modify it if you want, and put it at the top of the code. I want to say that this is my first pull request so I am open to receive every type of advice. Do you know why the code didn't pass the benchmark test ?

@user1:
The error says:
```
Couldn't load asv.plugins._mamba_helpers because
No module named 'libmambapy'
```
Therefore, I don't think is due to our changes. Try to re-run just this workflow manually if you can. Otherwise I cannot help you. I'm not an expert of libmambapay.

About the import. I will put the import at the beginning of the script because is the place where people are expecting imports. You import in the middle of a script just in special cases.


@author:
Thank you for your feedback, @user1. I appreciate your review. I'll implement your suggestion by moving the import statement to the beginning of the script for better clarity and conformity with best practices. Additionally, I'll rerun the test to verify if the changes resolve the issue. If there are any further concerns or recommendations, please don't hesitate to share them.