Remove unnessary numpy lookups for int, float, complex, bool and str #6603

MSeifert04 · 2017-09-24T20:54:42Z

The np.int, np.float, etc. are just aliases for the Python builtins so we really shouldn't be using them. Note that NumPy recently did the same (based on this issue).

The aliases are:

np.object
np.str
np.int
np.float
np.complex
np.bool

But not the ones that end with an underscore (np.float_) or a number (np.int32) or np.intp and np.intc which are actually different.

Note that the PR probably fixed "too much". I just replaced the np. in front of these or removed it altogether where it didn't make sense. However, in some cases it could just be that the author wanted to use np.*_ with the trailing underscore. So this probably requires a careful review.

Checklist:

astropy-bot · 2017-09-24T20:54:56Z

Hi there @MSeifert04 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here

mhvk · 2017-09-24T21:24:37Z

Now at the right PR: @MSeifert04 - I didn't look at everything, but time and units are definitely OK.

MSeifert04 · 2017-09-25T13:48:06Z

astropy/convolution/tests/test_kernel_class.py



-NUMS = [1, 1., np.float(1.), np.float32(1.), np.float64(1.)]
+NUMS = [1, 1., np.float32(1.), np.float64(1.)]


This is one of the cases where I wasn't sure what was intended. Given that np.float(1.) is identical to 1. I simply removed it.

MSeifert04 · 2017-09-25T13:50:01Z

astropy/table/tests/test_column.py

        ndarray or MaskedArray, even in the case of a multi-dim column.
        """
-        integer_types = (int, np.int)
+        integer_types = [int]


This one was a bit weird. Was it indentded to test np.int_?

I looked back at #3095, and, yes, this is definitely meant to test all possible integer types, so replace this with np.int_. (The actual code looks for subclasses of int and np.integer)

pllim · 2017-09-25T18:11:08Z

This touches too many sub-packages of various functionalities. I recommend that we keep a check list of sub-packages being touched and whether each is approved by its lead/deputy maintainer; Just to be on the safe side.

MSeifert04 · 2017-09-25T18:14:40Z

Actually the changes don't change anything, except that they removes some obvious redundancy. But a checklist is probably in order. 😄

pllim · 2017-09-25T18:15:55Z

Just want to ensure due diligence to ensure that the redundancy wasn't added on purpose. But if I am not making sense, then feel free to ignore. 😅

MSeifert04 · 2017-09-25T18:20:56Z

Well, given that np.int is int it probably doesn't make sense to have redundancy (test for int and np.int). But it's always possible that the original author wanted to test np.int_ which would be different.

However it's impossible to be really sure on that. But I can re-add the redundancy so the PR doesn't change anything except for the np lookup.

MSeifert04 · 2017-09-25T18:25:15Z

astropy/utils/misc.py

        if isinstance(obj, (np.ndarray, np.number)):
            return obj.tolist()
-        elif isinstance(obj, (complex, np.complex)):
+        elif isinstance(obj, (complex, np.complexfloating)):


Not sure if that check is correct or if I should rather have removed the second type.

Oh, dear. I added this in #552 to support Cone Search. I'll have to find time to dig through the 181 comments to see why I wrote this the way I did...

Nevermind, on second thought I should just remove the second type. Currently it was only checking for complex, complex. But np.complexfloating would be already catched by the np.number isinstance check (np.complexfloating is a subclass of np.number) above so it's likewise a no-op.

MSeifert04 · 2017-09-25T18:25:31Z

astropy/utils/tests/test_data_info.py

               ('U4', STRING_TYPE_NAMES[(True, 'U')] + '4'),
               (np.void, 'void'),
               (np.int32, 'int32'),
-               (np.bool, 'bool'),


This was identical to the next line so I removed it.

MSeifert04 · 2017-09-25T18:29:38Z

astropy/table/column.py


      - Python non-string type (float, int, bool)
-      - Numpy non-string type (e.g. np.float32, np.int64, np.bool)
+      - Numpy non-string type (e.g. np.float32, np.int64, bool)


Maybe this should've been np.bool_ given that it refers to the NumPy type.

MSeifert04 · 2017-09-25T19:37:55Z

I think I commented all non-trivial changes made here. The other changes were rather trivial: dtype specifiers for arrays and conversions. Maybe that makes reviewing it easier. :)

mhvk

@MSeifert04 - coordinates is all OK, and for cosmology most of the dtype can be removed (those in tests are OK).

mhvk · 2017-09-27T17:56:29Z

astropy/cosmology/core.py


        # Tcmb may have units
-        self._Tcmb0 = u.Quantity(Tcmb0, unit=u.K, dtype=np.float)
+        self._Tcmb0 = u.Quantity(Tcmb0, unit=u.K, dtype=float)


This can be removed altogether (the default is float64, which is what one would want here).

mhvk · 2017-09-27T17:56:44Z

astropy/cosmology/core.py


        # Hubble parameter at z=0, km/s/Mpc
-        self._H0 = u.Quantity(H0, unit=u.km / u.s / u.Mpc, dtype=np.float)
+        self._H0 = u.Quantity(H0, unit=u.km / u.s / u.Mpc, dtype=float)


Here dtype can be removed as well.

mhvk · 2017-09-27T17:56:58Z

astropy/cosmology/core.py

            # Only massless
            return u.Quantity(np.zeros(self._nmasslessnu), u.eV,
-                              dtype=np.float)
+                              dtype=float)


mhvk · 2017-09-27T17:57:07Z

astropy/cosmology/core.py

            # Only massive
            return u.Quantity(self._massivenu_mass, u.eV,
-                              dtype=np.float)
+                              dtype=float)


And here...

mhvk · 2017-09-27T17:57:16Z

astropy/cosmology/core.py

        numass = np.append(np.zeros(self._nmasslessnu),
                           self._massivenu_mass.value)
-        return u.Quantity(numass, u.eV, dtype=np.float)
+        return u.Quantity(numass, u.eV, dtype=float)


And here again.

mhvk · 2017-09-27T17:58:17Z

astropy/cosmology/core.py

            z = np.asarray(z)
            if self._Onu0 == 0:
-                return np.zeros(np.asanyarray(z).shape, dtype=np.float)
+                return np.zeros(np.asanyarray(z).shape, dtype=float)


Remove dtype here too.

mhvk · 2017-09-27T17:58:31Z

astropy/cosmology/core.py

            else:
                return prefac * self._Neff *\
-                    np.ones(np.asanyarray(z).shape, dtype=np.float)
+                    np.ones(np.asanyarray(z).shape, dtype=float)


For ones, float is also the default.

mhvk · 2017-09-27T17:59:14Z

astropy/cosmology/core.py

            return -1.0
        else:
-            return -1.0 * np.ones(np.asanyarray(z).shape, dtype=np.float)
+            return -1.0 * np.ones(np.asanyarray(z).shape, dtype=float)


mhvk · 2017-09-27T17:59:20Z

astropy/cosmology/core.py

            return 1.
        else:
-            return np.ones(np.asanyarray(z).shape, dtype=np.float)
+            return np.ones(np.asanyarray(z).shape, dtype=float)


mhvk · 2017-09-27T17:59:30Z

astropy/cosmology/core.py

            return self._w0
        else:
-            return self._w0 * np.ones(np.asanyarray(z).shape, dtype=np.float)
+            return self._w0 * np.ones(np.asanyarray(z).shape, dtype=float)


MSeifert04 · 2017-09-28T11:37:36Z

@taldcroft Could you have a look?

Especially the table test which checked int and np.int was a bit strange.

mhvk

I looked through all remaining stuff and think it is OK modulo the comments for table and io.misc. I think this is ready to go in with those taken into account.

mhvk · 2017-09-28T14:13:05Z

astropy/table/tests/test_column.py

        ndarray or MaskedArray, even in the case of a multi-dim column.
        """
-        integer_types = (int, np.int)
+        integer_types = [int]


I looked back at #3095, and, yes, this is definitely meant to test all possible integer types, so replace this with np.int_. (The actual code looks for subclasses of int and np.integer)

mhvk · 2017-09-28T14:14:57Z

astropy/utils/iers/iers.py


        # See explanation in _refresh_table_as_needed for these conditions
        auto_max_age = (conf.auto_max_age if conf.auto_max_age is not None
-                        else np.finfo(np.float).max)


This one is OK.

mhvk · 2017-09-28T14:20:48Z

astropy/convolution/tests/test_kernel_class.py



-NUMS = [1, 1., np.float(1.), np.float32(1.), np.float64(1.)]
+NUMS = [1, 1., np.float32(1.), np.float64(1.)]


mhvk · 2017-09-28T14:26:09Z

astropy/io/misc/tests/test_hdf5.py

 ALL_DTYPES = [np.uint8, np.uint16, np.uint32, np.uint64, np.int8,
              np.int16, np.int32, np.int64, np.float32, np.float64,
-              np.bool, '|S3']
+              bool, '|S3']


It doesn't really matter, but I think this more logically is np.bool_ (or is alias, np.bool8)

mhvk · 2017-09-28T14:36:28Z

astropy/io/misc/tests/test_yaml.py

-                               np.float(2.0), np.float64(),
-                               np.complex(3, 4), np.complex_(3 + 4j),
+                               2.0, np.float64(),
+                               complex(3, 4), np.complex_(3 + 4j),


Maybe just 3 + 4j?

mhvk · 2017-09-28T14:38:49Z

astropy/io/misc/yaml.py

 AstropyDumper.add_representer(np.bool_,
                              yaml.representer.SafeRepresenter.represent_bool)
-for np_type in [np.int_, np.int, np.intc, np.intp, np.int8, np.int16, np.int32,
+for np_type in [np.int_, int, np.intc, np.intp, np.int8, np.int16, np.int32,


I think here int can be removed, since yaml already has it built in.

mhvk · 2017-09-28T14:38:56Z

astropy/io/misc/yaml.py

    AstropyDumper.add_representer(np_type,
                                 yaml.representer.SafeRepresenter.represent_int)
-for np_type in [np.float_, np.float, np.float16, np.float32, np.float64,
+for np_type in [np.float_, float, np.float16, np.float32, np.float64,


Same for float

mhvk · 2017-09-28T14:39:16Z

astropy/io/misc/yaml.py

    AstropyDumper.add_representer(np_type,
                                 yaml.representer.SafeRepresenter.represent_float)
-for np_type in [np.complex_, np.complex, np.complex64, np.complex128]:
+for np_type in [np.complex_, complex, np.complex64, np.complex128]:


But not for complex since this is a custom representer.

taldcroft · 2017-09-28T15:43:30Z

Well I admit I did not realize that int and np.int (etc) were the same thing. Is there a real performance driver for doing this PR? I imagine in most cases in the code these lookups are not in an inner loop and the extra 30 ns is not actually affecting anything. In that case my general attitude would be to leave well enough alone since testing is not always perfect.

MSeifert04 · 2017-09-28T15:50:22Z

Well I admit I did not realize that int and np.int (etc) were the same thing.

Yes, that's the main driver for the PR. It's not actually about the lookup cost or if NumPy will ever deprecate them (probably not). However as can be seen by the existing "complicated" cases it has been leading to unintended behavior, almost "problems". For example testing the wrong thing, overwriting the YAML writer for plain ints and such like.

mhvk · 2017-09-28T15:56:05Z

Agreed with @MSeifert04 here - I didn't know either that np.float is float - and I think it is worthwhile just for our code not reinforcing that misconception! (And this is the right time to do it, with nearly every file touched anyway for the python2 removal...)

It is unnecessary because it subclasses np.number which would go into the if above. Also use np.bool_ in table column docs because it explicitly lists it as NumPy type.

taldcroft · 2017-09-28T17:35:35Z

OK, fair enough.

mhvk · 2017-09-28T18:07:29Z

OK, this seems all done, so merging. Thanks, @MSeifert04!

MSeifert04 · 2017-09-28T20:02:05Z

Thanks everyone for the reviews and comments. 👍

As discussed in astropy#6603 (and numpy/numpy#6103) it's unnecessary and misleading to use the Python types from the NumPy namespace. With this PR this uses the default NumPy dtype corresponding to these Python types.

…py_lookup Remove unnessary numpy lookups for int, float, complex, bool and str

MSeifert04 added the Refactoring label Sep 24, 2017

MSeifert04 added this to the v2.0.3 milestone Sep 24, 2017

MSeifert04 added the no-changelog-entry-needed label Sep 24, 2017

MSeifert04 modified the milestones: v2.0.3, v3.0.0 Sep 24, 2017

MSeifert04 commented Sep 25, 2017

View reviewed changes

pllim added convolution coordinates cosmology io.ascii io.fits io.misc modeling nddata stats table time units wcs labels Sep 25, 2017

MSeifert04 commented Sep 25, 2017

View reviewed changes

mhvk reviewed Sep 27, 2017

View reviewed changes

mhvk reviewed Sep 28, 2017

View reviewed changes

MSeifert04 added 12 commits September 28, 2017 18:06

remove unnecessary float lookups from numpy module

6d8868c

remove unnecessary int lookups from numpy module

35aed89

remove unnecessary str lookups from numpy module

bb03652

remove unnecessary bool lookups from numpy module

ebd0477

remove unnecessary complex lookups from numpy module

e301cba

fixed failing test

c3be54d

Also remove np.float from pyx files.

b7fdb18

remove unnecessary object lookups from numpy module

345554f

undo one change that won't work correctly anymore (soon-ish)

a3b0055

Remove np.complexfloating from isinstance check again.

5b308e7

It is unnecessary because it subclasses np.number which would go into the if above. Also use np.bool_ in table column docs because it explicitly lists it as NumPy type.

remove dtype=float when it's already the default

c82f4a7

adress mhvks comments

378622d

mhvk merged commit 9e64eb0 into astropy:master Sep 28, 2017

MSeifert04 deleted the unnessary_numpy_lookup branch September 28, 2017 23:17

This was referenced Nov 4, 2019

Maintenance: Don't use NumPys re-exported Python built-ins. #9526

Merged

Fix input data type validation for Bayesian Blocks #9513

Merged

pllim mentioned this pull request Nov 27, 2019

Is change in behavior of quantity.value for scalars deliberate? #9697

Closed

bsipocz mentioned this pull request Dec 13, 2019

Fix skybot unumbered asteroid return astropy/astroquery#1598

Merged

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 7, 2020

Merge pull request astropy/astropy#6603 from MSeifert04/unnessary_num…

3cad100

…py_lookup Remove unnessary numpy lookups for int, float, complex, bool and str

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 8, 2020

Merge pull request astropy/astropy#6603 from MSeifert04/unnessary_num…

d2efd46

…py_lookup Remove unnessary numpy lookups for int, float, complex, bool and str



		NUMS = [1, 1., np.float(1.), np.float32(1.), np.float64(1.)]
		NUMS = [1, 1., np.float32(1.), np.float64(1.)]

Uh oh!

Remove unnessary numpy lookups for int, float, complex, bool and str #6603

Remove unnessary numpy lookups for int, float, complex, bool and str #6603

Uh oh!

Conversation

MSeifert04 commented Sep 24, 2017 • edited by bsipocz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot bot commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Sep 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Sep 25, 2017

Uh oh!

MSeifert04 commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Sep 25, 2017

Uh oh!

MSeifert04 commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Sep 25, 2017

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Sep 28, 2017

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Sep 24, 2017 •

edited by bsipocz

Loading

astropy-bot bot commented Sep 24, 2017 •

edited

Loading

MSeifert04 commented Sep 25, 2017 •

edited

Loading

pllim Sep 25, 2017 •

edited

Loading

MSeifert04 Sep 25, 2017 •

edited

Loading