Change "hasattr(x, '__call__')" to "callable(x)" etc

[jhpalmieri]

Make three changes:

• hasattr(x, '__call__') -> callable(x)
• hasattr(x, '__iter__') -> instance(x, Iterable)
• hasattr(x, '__getitem__') -> isinstance(x, (Sequence, Mapping))

where Iterable, Mapping, Sequence are imported from collections.abc. See the more recent (Python 3 specific) answers at https://stackoverflow.com/questions/1952464/in-python-how-do-i-determine-if-an-object-is-iterable and https://stackoverflow.com/questions/25231989/how-to-check-if-a-variable-is-a-dictionary-in-python.

Component: python3

Author: John Palmieri

Branch/Commit: 12a48e6

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33451

[jhpalmieri]

Branch: u/jhpalmieri/no-hasattr-call

[jhpalmieri]

Commit: 4b92197

[jhpalmieri]

New commits:

2eae592	change "hasattr(x, '__call__')" to "callable(x)"
4b92197	Change "hasattr(x, '__iter__')" to "isinstance(x, Iterable)",

[mkoeppe]

Description changed:

--- 
+++ 
@@ -2,7 +2,7 @@

 - `hasattr(x, '__call__')` -> `callable(x)`
 - `hasattr(x, '__iter__')` -> `instance(x, Iterable)`
-- `hasattr(x, '__item__')` -> `isinstance(x, Mapping)`
+- `hasattr(x, '__getitem__')` -> `isinstance(x, Mapping)`

 where `Iterable` and `Mapping` are imported from `collections.abc`. See 
 the more recent (Python 3 specific) answers at https://stackoverflow.com/questions/1952464/in-python-how-do-i-determine-if-an-object-is-iterable and https://stackoverflow.com/questions/25231989/how-to-check-if-a-variable-is-a-dictionary-in-python.

[mkoeppe]

comment:4

Not sure about this change; should it really iterate over the keys of a dictionary?

--- a/src/sage/geometry/hyperbolic_space/hyperbolic_point.py
+++ b/src/sage/geometry/hyperbolic_space/hyperbolic_point.py
@@ -60,6 +60,7 @@ Some more examples::
 #                  http://www.gnu.org/licenses/
 #***********************************************************************

+from collections.abc import Iterable, Mapping
 from sage.structure.element import Element
 from sage.structure.richcmp import richcmp, op_NE
 from sage.symbolic.constants import I
@@ -532,7 +533,8 @@ class HyperbolicPoint(Element):
         else:  # It is an interior point
             if p in RR:
                 p = CC(p)
-            elif hasattr(p, 'items') or hasattr(p, '__iter__'):
+            elif isinstance(p, (Iterable, Mapping)):
+                # p is iterable or a dict (or similar).
                 p = [numerical_approx(k) for k in p]
             else:
                 p = numerical_approx(p)

[mkoeppe]

comment:5

Also Mapping is not the only ABC that provides __getitem__, also Sequence does https://docs.python.org/3/library/collections.abc.html

[jhpalmieri]

comment:6

Replying to @mkoeppe:

Not sure about this change; should it really iterate over the keys of a dictionary?

That's not a change, though.

[jhpalmieri]

comment:7

Replying to @mkoeppe:

Also Mapping is not the only ABC that provides __getitem__, also Sequence does https://docs.python.org/3/library/collections.abc.html

We could try isinstance(x, dict) instead.

[mkoeppe]

comment:8

Is there a doctest that illustrates this use? It seems rather strange to me

[jhpalmieri]

comment:9

I didn't mean to make the change

-                    elif hasattr(other._deepcopy_labels_, '__getitem__'):
+                    elif isinstance(other._deepcopy_labels_, Mapping):

in finite_state_machine.py. I'll undo that one.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4b92197 to 69173e9

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

69173e9

Change "hasattr(x, '__iter__')" to "isinstance(x, Iterable)",

[jhpalmieri]

comment:11

Replying to @mkoeppe:

Is there a doctest that illustrates this use? It seems rather strange to me

It doesn't look like it, since that method doesn't work for a tuple, let alone a dict:

sage: p1 = UHP.get_point(.2 + .3*I)
sage: p1.coordinates()
0.200000000000000 + 0.300000000000000*I
sage: p1.show()
Launched png viewer for Graphics object consisting of 2 graphics primitives
sage: p2 = UHP.get_point((.2, .3))
sage: p2.coordinates()
(0.200000000000000, 0.300000000000000)
sage: p2.show()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: unsupported operand parent(s) for +: 'Vector space of dimension 2 over Real Field with 53 bits of precision' and 'Symbolic Ring'

[jhpalmieri]

comment:12

I guess that's not relevant. A modification of an existing doctest

sage: HyperbolicPlane().KM().get_point((0,0)).show()

namely replacing (0,0) by [0,0], would use that code. I am not particularly happy with the following but I don't want to put a lot of work into this particular module:

sage: p1 = HyperbolicPlane().KM().get_point((0,1/2))
sage: p2 = HyperbolicPlane().KM().get_point([0,1/2])
sage: p1 == p2
False
sage: p1
Point in KM (0, 1/2)
sage: p2
Point in KM [0, 1/2]
sage: p1.coordinates()
(0, 1/2)
sage: type(p1.coordinates())
<class 'sage.modules.vector_rational_dense.Vector_rational_dense'>
sage: p2.coordinates()
[0, 1/2]
sage: type(p2.coordinates())
<class 'list'>

It would be better if the input were normalized: any iterable converted to a vector. Maybe this would help:

diff --git a/src/sage/geometry/hyperbolic_space/hyperbolic_point.py b/src/sage/geometry/hyperbolic_space/hyperbolic_point.py
index 5ad5b5342c..c386f0057b 100644
--- a/src/sage/geometry/hyperbolic_space/hyperbolic_point.py
+++ b/src/sage/geometry/hyperbolic_space/hyperbolic_point.py
@@ -193,6 +193,10 @@ class HyperbolicPoint(Element):

             sage: p = HyperbolicPlane().UHP().get_point(I)
             sage: TestSuite(p).run()
+            sage: p1 = HyperbolicPlane().KM().get_point((0,0))
+            sage: p2 = HyperbolicPlane().KM().get_point([0,0])
+            sage: p1 == p2
+            True
         """
         if is_boundary:
             if not model.is_bounded():
@@ -206,7 +210,7 @@ class HyperbolicPoint(Element):
                 "{0} is not a valid".format(coordinates) +
                 " point in the {0} model".format(model.short_name()))

-        if isinstance(coordinates, tuple):
+        if isinstance(coordinates, Iterable):
             coordinates = vector(coordinates)
         self._coordinates = coordinates
         self._bdry = is_boundary

Then I would be tempted to delete the elif isinstance(p, (Iterable, Mapping)) block.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 69173e9 to c731d4e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

c731d4e

trac 33451: treat all iterables the same when constructing hyperbolic points

[mkoeppe]

comment:14

Yes, I think this is a good change

[mkoeppe]

Description changed:

--- 
+++ 
@@ -2,7 +2,7 @@

 - `hasattr(x, '__call__')` -> `callable(x)`
 - `hasattr(x, '__iter__')` -> `instance(x, Iterable)`
-- `hasattr(x, '__getitem__')` -> `isinstance(x, Mapping)`
+- `hasattr(x, '__getitem__')` -> `isinstance(x, (Sequence, Mapping))`

-where `Iterable` and `Mapping` are imported from `collections.abc`. See 
+where `Iterable`, `Mapping`, `Sequence` are imported from [collections.abc](https://docs.python.org/3/library/collections.abc.html). See 
 the more recent (Python 3 specific) answers at https://stackoverflow.com/questions/1952464/in-python-how-do-i-determine-if-an-object-is-iterable and https://stackoverflow.com/questions/25231989/how-to-check-if-a-variable-is-a-dictionary-in-python.

[jhpalmieri]

comment:16

Note that my changes replace hasattr(x, 'items'), not hasattr(x, '__getitem__'). We can do the second one, but that wasn't part of the original plan. I did the first replacement because of the comment "# data is a dict (or something similar)" after one of those tests, so it was clear why they were testing hasattr(x, 'items').

[mkoeppe]

comment:17

Ah, now I get what items was supposed to do. Thanks

[mkoeppe]

comment:18

There are also a small number of hasattr(..., '__len__') in the library

[mkoeppe]

comment:19

But all of this can be taken care of in a follow-up ticket.

The present branch looks good and passes tests.

[mkoeppe]

Reviewer: Matthias Koeppe

[jhpalmieri]

comment:20

Thanks!

By the way, among other things I see:

• hasattr(..., '__len__')
• hasattr(..., '__getitem__')
• hasattr(..., '__dict__')
• hasattr(..., '__contains__')
• hasattr(..., '__classcall__')

I'm not sure what these are trying to accomplish. Maybe we should document some of this in the developer's guide: "A good way to test [or The preferred way to test"] if an object is a dictionary/iterable/whatever is ...".

[vbraun]

comment:21

Merge failure on top of:

70aa673e95 Trac #33447: Doctest failure on ubuntu-jammy because system Singular is accepted

503795926f Trac #33452: adjust error messages in topology/

599d44584f Trac #33449: Move plotting and affine hull to Polyhedron_base6

719453b6a6 Trac #33443: slow doctest improvements (isogeny_small_degree, function_field, doctest/test.py)

5d33ab9323 Trac #33442: Remove package ratpoints deprecated 4 years ago

9550637260 Trac #33435: Move complex number powering test to correct location

397ca79b4b Trac #33355: Generate coverage results and upload to codecov

a8777acf47 Trac #33223: make PARI the default for EllipticCurve_field.weierstrass_p()

c7adc6cc57 Trac #33112: some speedups in AdditiveAbelianGroupWrapper.discrete_log()

8230df5b38 Trac #32770: Expose and access tachyon command line flags

135c8e5bb1 Trac #31426: Apply flat shading when plotting 3d polyhedra with Three.js

32e26f275f Trac #24536: find_local_maximum/minimum() fails with expressions containing complex numbers

c0b15ffde0 Trac #33450: Improve vs code config

a76af34b81 Trac #33312: WeierstrassIsomorphism.degree() returns a Python int

8368718592 Trac #33094: update URLs in SPKG.rst for ECL

4f76f139db Trac #32922: Change parameter name

44b5d43de7 Trac #32384: make some AdditiveAbelianGroupWrapper methods visible

232f88fe88 Trac #32251: Fix doctest in the "Y pentamino" example from Donald Knuth

6adf64eec4 Trac #30999: expose method hyperbolicity in graphs

14e3e755d2 Trac #25787: Remove documentation configuration from micro_release

87d5c62133 Trac #33414: Remove sage-open

c254f33235 Trac #33413: Remove sage-native-execute

6dcb3dc676 Trac #33412: Remove sage-update-src

aeb3fb2594 Trac #33411: include time for checking doctest output in '--warn-long'

b20aeb7ee9 Trac #33322: Several ImageMagick tests need "long time"

db459d4eb3 Trac #33318: speed up is_prime() for very small integers

0df0bad12f Trac #33308: Remove expired deprecations that use lazy_import(MODULE, "*")

7c9535ef80 Trac #33292: mark invlex ordering as global

e6d8672713 Trac #33286: Put topology in the reference manual

6e45b9e469 Trac #33270: Difference between a formula and an image in Contour Plots

895ded3c44 Trac #33224: PARI-backed LaurentSeries truncates exact polynomials to default precision

eeda513423 Trac #33434: Fix pyflake in sage.graphs.graph

f45550a9d3 Trac #33424: pep8 for function field maps

9553cc37d8 Trac #33319: polynomial rings should not attempt to use Singular in characteristic >= 2^31

13d038fd9f Trac #33147: use PARI's ellmul() for elliptic curves over finite fields

0c85ad2f59 Trac #33433: Correct order of parameters in min_spanning_tree

8e935af159 Trac #33421: Add elliptic_curves dependency to sagemath_doc_html

111bcfa1f7 Trac #33216: reduce precision for .formal() in EllipticCurveIsogeny.dual()

195cf6ab4d Trac #33215: WeierstrassIsomorphism should coerce its inputs to a common parent

df9c2612a6 Trac #33210: Fix Cython warnings in sage.rings.polynomial.polynomial_modn_dense_ntl

ac61ea8b62 Trac #33208: Remove unused code from sage/rings/padics/pow_computer_ext.pyx

bc069a7cb2 Trac #33193: Fix Cython incompatible redefinition warnings in padics

8a21049265 Trac #33186: Fix "referenced before assignment" warnings in padics

490fdfded3 Trac #33055: conda-forge: Fix build of python3 spkg

beedaabc7e Trac #32997: docker image fails to build on tigerlake

28f2f9a48b Trac #32786: opportunistic caching of elliptic-curve and point orders

1ac2b995a4 Trac #12419: factorization of 0 in GF(p)[x,y] fails

9349a36d34 Trac #33429: some cleanup in toric schemes

df9d1d77aa Trac #33427: numerical noise in effective_resistance involving RDF

c6f2af1800 Trac #33425: adjust error messages in combinat/words

449c7f1d66 Trac #33423: Cancel the linter

b30c72fe86 Trac #33420: fix E111 (indentation) in quadratic_forms

cc8a39082b Trac #33419: fix E111 (indentation) in manifolds, stats, finance

6410a7cd86 Trac #33415: fix the linter

ec318c0d08 Trac #33410: Polyhedron_base.volume(engine='lrs', measure='induced')

b6fecc7464 Trac #33399: Bug in ExpressionNice with composite variables

6779f414bf Trac #33363: (Too) long doctest in sage/matrix/matrix_integer_dense.pyx

9e90afaba0 Trac #29215: valuation error

2a16069f4c Trac #29114: Only ./bootstrap should glob build/pkgs

062a90c9d1 Trac #33403: Polyhedron._test_product, _test_dilation: Skip tests if test prereqs cannot be imported

cc4ffb3fa1 Trac #33402: sage.geometry.polyhedron: More # optional

9333fa5dc3 Trac #33395: document using Wolfram Engine (a.k.a. wolframscript) with Sage

f346730807 Trac #33394: correct docs for running notebook on a specific port

1e8ba0aac4 Updated SageMath version to 9.6.beta3

merge was not clean: conflicts in src/sage/plot/misc.py

[fchapoton]

Changed branch from u/jhpalmieri/no-hasattr-call to public/ticket/33451

[fchapoton]

comment:22

after trivial rebase, setting back to positive

---

New commits:

12a48e6

Merge branch 'u/jhpalmieri/no-hasattr-call' in 9.6.b5

[fchapoton]

Changed commit from c731d4e to 12a48e6

[jhpalmieri]

comment:23

Thank you for taking care of that. I didn't get the email notification about the merge failure so I didn't even know there was a problem.

[vbraun]

Changed branch from public/ticket/33451 to 12a48e6