Replace hasattr(... '__iter__') by check with iterable

[theHamsta]

Minor code clean-up.

References to other Issues or PRs

I found that Sympy sometimes checks for objects having the __iter__ attribute while there is some ways to mark your object as not iterable (#19772). However, such checks are not performed when just checking for __iter__.

Brief description of what is fixed or changed

Checking for attribute __iter__ circumvents the Sympy way of checking if something is iterable

Other comments

Release Notes

NO ENTRY

[sympy-bot]

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

• No release notes entry will be added for this pull request.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->
Minor code clean-up.

#### References to other Issues or PRs
I found that Sympy sometimes checks for objects having the `__iter__` attribute while there is some ways to mark your object as not iterable (#19772). However, such checks are not performed when just checking for `__iter__`. 

#### Brief description of what is fixed or changed
Checking for attribute `__iter__` circumvents the Sympy way of checking if something is iterable

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

[oscarbenjamin]

Is there a way to test the effect of this change?

[theHamsta]

You can call poly(foo + 1, foo) where foo is has __iter__ but is not iterable by the definition of Sympy. Can add this later.

[oscarbenjamin]

It would be good to have a test for the change.

It seems there are lots of places where this happens:

$ git grep __iter__ | grep has
sympy/combinatorics/permutations.py:    the __iter__ method has been redefined to give the array form of the
sympy/core/compatibility.py:# hasattr(obj, "__iter__") behaves differently in Python 2 and Python 3.  In
sympy/core/compatibility.py:# particular, hasattr(str, "__iter__") is False in Python 2 and True in Python 3.
sympy/core/multidimensional.py:        if hasattr(x, "__iter__"):
sympy/core/multidimensional.py:        if hasattr(i, "__iter__"):
sympy/core/multidimensional.py:                if hasattr(entry, "__iter__"):
sympy/holonomic/holonomic.py:            if hasattr(limits, "__iter__"):
sympy/holonomic/holonomic.py:        if hasattr(limits, "__iter__"):
sympy/holonomic/holonomic.py:        if not hasattr(points, "__iter__"):
sympy/polys/constructor.py:    if hasattr(obj, '__iter__'):
sympy/polys/constructor.py:    if hasattr(obj, '__iter__'):
sympy/polys/domains/domain.py:        if hasattr(symbols, '__iter__'):
sympy/polys/numberfields.py:    if hasattr(extension, '__iter__'):
sympy/polys/polyfuncs.py:    if not hasattr(F, '__iter__'):
sympy/polys/polyoptions.py:        elif len(gens) == 1 and hasattr(gens[0], '__iter__'):
sympy/polys/polyoptions.py:            if not hasattr(extension, '__iter__'):
sympy/polys/polyoptions.py:        if hasattr(symbols, '__iter__'):
sympy/polys/polytools.py:    if hasattr(f, '__iter__'):
sympy/polys/polytools.py:    if hasattr(f, '__iter__'):
sympy/polys/polytools.py:    elif hasattr(expr, '__iter__'):
sympy/polys/polytools.py:    if not hasattr(F, '__iter__'):
sympy/polys/polyutils.py:    if len(gens) == 1 and hasattr(gens[0], '__iter__'):
sympy/printing/repr.py:        elif hasattr(expr, "args") and hasattr(expr.args, "__iter__"):
sympy/simplify/epathtools.py:                elif hasattr(expr, '__iter__'):
sympy/simplify/epathtools.py:                elif hasattr(expr, '__iter__'):
sympy/solvers/solveset.py:    if hasattr(symbols[0], '__iter__'):
sympy/solvers/solveset.py:    if symbols and hasattr(symbols[0], '__iter__'):
sympy/solvers/solveset.py:    if hasattr(system, '__iter__'):
sympy/solvers/solveset.py:    if hasattr(symbols[0], '__iter__'):
sympy/stats/tests/test_rv.py:            hasattr(obj, '__iter__') and
sympy/utilities/iterables.py:        elif hasattr(expr, "__iter__"):
sympy/utilities/lambdify.py:    if isinstance(modules, (dict, str)) or not hasattr(modules, '__iter__'):

Some of those should probably be something like if isinstance(obj, Tuple) etc.

[oscarbenjamin]

Another possibility is to use is_sequence.

[czgdp1807]

@theHamsta Would you like to complete this PR? Please do not close this. Thanks for your contributions.

[theHamsta]

Hi. I can finish this. But maybe only next weekend. I could try to replace all the occurrences that oscarbenjamin highlighted.

[oscarbenjamin]

It's not necessary to change all of those. What does need changing in this PR is that there should probably be a test.

Presumably you had a reason for wanting to make this change so what exactly is fixed by it?

Or is this more of a style change?

[theHamsta]

I used a object that was iterable by Python criteria but marked as NotIterable (see linked issue). It was handled by most parts of SymPy but not consistently by all. iterable allows also having an __iter__ method that raises an error to judge not iterable.

The problem is basically that Sympy checks for iterablity in different ways. This is more a style change to prefer iterable to a raw check of __iter__.

[czgdp1807]

I think we shouldn't make style changes like these unless we are compelled to do so. I am suggesting this because, making style changes can lead to some unintended test failures or code breakage for the end users. Things are working now so let them work. Though if it fixes some issue/bug then we can surely conclude this PR. For now I am marking it as Could Close PR. Thanks for your contributions.

[theHamsta]

It's not a style change. it's a bug when sympy uses different methods to check for iterable. User objects will be iterable in some parts of sympy and not in others. Users should have unified

But a proper PR should unify it in whole sympy. so feel free to close.

[czgdp1807]

I see. Could you please share an example of such a scenario related to this PR where the current version 1.7(or any version) gives unwanted or incorrect results? It would be helpful to open an issue for this. Thanks.

[theHamsta]

See this discussion in a real life application: https://i10git.cs.fau.de/pycodegen/pystencils/-/merge_requests/166 I guess a mini-example would loose the context of why some would do that.

I guess it boils down to "What should users do to make something not Iterable?"

• inherit NonIterable -> does not work yet with sympy's iterable (could be fixed by NotIterable should always be respected #19772), does not work here
• have __iter__ throw an excecption -> does work with sympy's iterable, does work with Python iterable, does not work here with check for __iter__
• not have __iter__, but __getitem__ -> does not work when sympy does the check with Python's iterable

Best solution we found now:

• not have __iter__, and have a member _iterable = False

Could be extended by #19772. Then, NotIterable would also work.

Look at these two tests. It creates objects that inherit from sympy.Symbol. The second one works by making the daughter class NotIterable, the later not. The rest of the application worked with making __iter__ throw.

from pystencils.session import *

from sympy import poly


def test_field_access_poly():
    dh = ps.create_data_handling((20, 20))
    ρ = dh.add_array('rho')
    rho = ρ.center
    a = poly(rho+0.5, rho)
    print(a)


def test_field_access_piecewise():
    dh = ps.create_data_handling((20, 20))
    ρ = dh.add_array('rho')
    pw = sp.Piecewise((0, 1 < sp.Max(-0.5, ρ.center+0.5)), (1, True))
    a = sp.simplify(pw)
    print(a)

[codecov]

Codecov Report

Merging #19777 (b1782d6) into master (425353e) will increase coverage by 0.017%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19777       +/-   ##
=============================================
+ Coverage   75.742%   75.760%   +0.017%     
=============================================
  Files          673       673               
  Lines       174499    174500        +1     
  Branches     41205     41205               
=============================================
+ Hits        132170    132202       +32     
+ Misses       36607     36575       -32     
- Partials      5722      5723        +1     

[czgdp1807]

@rlamy @asmeurer @oscarbenjamin Is this change good? The above comment by @theHamsta seems convincing to have this change in master.

[czgdp1807]

I hope this is ready to go now? Will merge after two days if no objections raised until then. @oscarbenjamin

[oscarbenjamin]

Looks good. Thanks!