Repository: sympy/sympy
Pull request: atoms() method return value updated
#### References to other Issues or PRs
Fixes #10152 and aims to complete the work started in 
PR #10246


#### Brief description of what is fixed or changed
--> Updated the atoms() method of class Basic to check if .args value of an object is empty or not to check if a sympy object is a leaf node or not.

#### Other comments
Let us consider an example to see how this PR has updated the .atoms() method. 
So, now we will define 3 basic sympy objects.

```
>>> b1 = Basic()
>>> b2 = Basic()
>>> b12 = Basic(b1, b2)
```

Now, before the fix, the output of ```b12.atoms()``` is:

```
>>> b12.atoms()
set()
```

After the fix, the output is:

```
>>> b12.atoms()
set({Basic()})
```
#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* core
  * Updated the definition of atoms method of class Basic
<!-- END RELEASE NOTES -->

Problem statement:
expr.atoms() should return objects with no args instead of subclasses of Atom
`expr.atoms()` with no arguments returns subclasses of `Atom` in `expr`. But the correct definition of a leaf node should be that it has no `.args`. 

This should be easy to fix, but one needs to check that this doesn't affect the performance.

Hints from issue discussion:
The docstring should also be updated. 

Hi, can i work on this?

Sure. Did you read https://github.com/sympy/sympy/wiki/Introduction-to-contributing? 

How should I remove .args? Should I try to remove ._args from object instance or add a new attribute to class Atom(), is_leave. Which when assigned as false, will raise attribute error on .args. Or if creating a new object, what attributes should it have?

I think you're misunderstanding the issue. The issue is not to remove .args. Indeed, every SymPy object should have .args in order to be valid. 

The issue is that the `atoms()` method currently uses `x.is_Atom` to check for "atomic" expressions (expressions with no subexpressions), but it really should be checking `not x.args`. It should be a simple one-line fix to the `atoms` function definition, but a new test should be added, and the full test suite run to make sure it doesn't break anything (`./bin/test` from the sympy directory). 

Okay. But, Basic() also return .args to be null. So will not that also appear in the result of .atoms()?

Yes, that's an example of an object with no args but that isn't a subclass of Atom. `atoms` should return that, because it's a leaf in the expression tree. 

Okay, but if I am understanding you correct, won't this test fail?
https://github.com/sympy/sympy/blob/master/sympy/core/tests/test_basic.py#L73

Yes, it would need to be changed. This is a slight redefinition of what `atoms` means (although hopefully not enough of a breaking behavior to require deprecation). 

Can you look over it once and look if it is okay?
https://github.com/sympy/sympy/pull/10246

Commit message: codegen: Added an updated to an existing test case

A test case in test_cnodes.py used a test which considered
the old definition of .atoms() method. This test case was
updated accordingly.

Patch to review:
diff --git a/sympy/codegen/tests/test_cnodes.py b/sympy/codegen/tests/test_cnodes.py
index 3050fffa1a2f..1aa9e4caac49 100644
--- a/sympy/codegen/tests/test_cnodes.py
+++ b/sympy/codegen/tests/test_cnodes.py
@@ -1,6 +1,6 @@
 from sympy.core.symbol import symbols
 from sympy.printing.ccode import ccode
-from sympy.codegen.ast import Declaration, Variable, float64, int64
+from sympy.codegen.ast import Declaration, Variable, float64, int64, String
 from sympy.codegen.cnodes import (
     alignof, CommaOperator, goto, Label, PreDecrement, PostDecrement, PreIncrement, PostIncrement,
     sizeof, union, struct
@@ -66,7 +66,7 @@ def test_sizeof():
     assert ccode(sz) == 'sizeof(%s)' % typename
     assert sz.func(*sz.args) == sz
     assert not sz.is_Atom
-    assert all(atom == typename for atom in sz.atoms())
+    assert sz.atoms() == set([String('unsigned int'), String('sizeof')])
 
 
 def test_struct():
diff --git a/sympy/core/basic.py b/sympy/core/basic.py
index 6866648d4e4b..a6bab380981c 100644
--- a/sympy/core/basic.py
+++ b/sympy/core/basic.py
@@ -503,11 +503,11 @@ def atoms(self, *types):
         if types:
             types = tuple(
                 [t if isinstance(t, type) else type(t) for t in types])
-        else:
-            types = (Atom,)
         result = set()
         for expr in preorder_traversal(self):
-            if isinstance(expr, types):
+            if not (types or expr.args):
+                result.add(expr)
+            elif isinstance(expr, types):
                 result.add(expr)
         return result
 
diff --git a/sympy/core/tests/test_basic.py b/sympy/core/tests/test_basic.py
index 50f8f9ee7b81..118e89db10f9 100644
--- a/sympy/core/tests/test_basic.py
+++ b/sympy/core/tests/test_basic.py
@@ -137,7 +137,7 @@ def test_subs_with_unicode_symbols():
 
 
 def test_atoms():
-    assert b21.atoms() == set()
+    assert b21.atoms() == set([Basic()])
 
 
 def test_free_symbols_empty():


Code review comments:
diff_hunk: @@ -503,11 +503,11 @@ def atoms(self, *types):
         if types:
             types = tuple(
                 [t if isinstance(t, type) else type(t) for t in types])
-        else:
-            types = (Atom,)
         result = set()
         for expr in preorder_traversal(self):
-            if isinstance(expr, types):
+            if not (types or expr.args):
+                result.add(expr)
+            elif isinstance(expr, types):
path: sympy/core/basic.py
text: @user1:
I think it would be clearer to write:
```python
nodes = preorder_traversal(self)
if types:
    result = {node for node in nodes if isinstance(node, types)}
else:
    result = {node for node in nodes if not node.args}
```

@author:
@user1 sure thing.