Construct 1d array from listlike #18769

toobaz · 2017-12-13T18:10:31Z

tests passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

#18626 isn't converging, will need more work and I won't have much time soon, so I took away the asv and refactoring that should be straighforward to merge.

asv says

       before           after         ratio
     [96439fb1]       [e72437c5]
-        38.6±1ms       35.1±0.5ms     0.91  panel_ctor.Constructors3.time_panel_from_dict_same_index
-      24.2±0.1μs       21.8±0.2μs     0.90  ctors.Constructors.time_index_from_array_string
-        39.7±1ms      34.9±0.02ms     0.88  panel_ctor.Constructors2.time_panel_from_dict_equiv_indexes
-     14.1±0.04μs      12.3±0.08μs     0.87  ctors.Constructors.time_series_from_ndarray
-      12.2±0.6μs      10.1±0.05μs     0.82  ctors.Constructors.time_dtindex_from_series

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

... but they are borderline, and don't always come up.

toobaz · 2017-12-14T07:06:48Z

@jreback Any hint about why all 2.7 fail with "ImportError: C extension: lib not built. If you want to import pandas from the source directory, you may need to run 'python setup.py build_ext --inplace --force' to build the C extensions first."?

codecov · 2017-12-14T08:17:49Z

Codecov Report

Merging #18769 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18769      +/-   ##
==========================================
- Coverage   91.64%   91.59%   -0.05%     
==========================================
  Files         154      154              
  Lines       51401    51430      +29     
==========================================
+ Hits        47106    47108       +2     
- Misses       4295     4322      +27

Flag	Coverage Δ
#multiple	`89.46% <100%> (-0.03%)`	⬇️
#single	`40.83% <80%> (-0.13%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/cast.py	`88.6% <100%> (+0.07%)`	⬆️
pandas/core/ops.py	`91.77% <100%> (ø)`	⬆️
pandas/core/common.py	`92.92% <100%> (-0.09%)`	⬇️
pandas/core/algorithms.py	`94.14% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`64.78% <0%> (-1.74%)`	⬇️
pandas/util/testing.py	`82.71% <0%> (-0.44%)`	⬇️
pandas/core/frame.py	`97.68% <0%> (-0.11%)`	⬇️
pandas/io/parsers.py	`95.55% <0%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a0ee19...e5b3cd5. Read the comment docs.

jorisvandenbossche · 2017-12-14T09:13:31Z

pandas/core/common.py

-                # we have a list-of-list
-                result[:] = [tuple(x) for x in values]
+        if hasattr(values, '__array__'):
+            values = [tuple(x) for x in values]


I don't understand this one (can you add some comments?)
The original comment said "we have a list-of-list", which is actually not possible because it was already checked before if it was a list.

Yes, that comment was wrong, and confusing. Will add a comment.

toobaz · 2017-12-14T11:18:02Z

Any hint about why all 2.7 fail with

Answering myself... it was a circular dependency

jreback

can you add some tests for construct_1d_object_array_from_listlike in test_cast.py (where similar things are tested).

jreback · 2017-12-14T11:19:55Z

asv_bench/benchmarks/ctors.py

@@ -33,3 +34,21 @@ def time_dtindex_from_series(self):

    def time_dtindex_from_index_with_series(self):
        Index(self.s)
+


can you use params for this

lots of examples in the other asvs

jreback · 2017-12-14T11:24:28Z

pandas/core/dtypes/cast.py

+
+    Returns
+    -------
+    1-dimensional numpy array of dtype "dtype"


I would rename this to
construct_1d_object_array_from_listlike and drop the dtype (always make it object).

you can also just add another function for this as well (e.g. have 1 that accepts the dtype as a required parameter ). I also don't think we are actually using dtype anywhere?

The array we return is of dtype dtype. So this will raise an error if e.g. dtype=int and the input contains lists, but this is good because it allows cleaner code (and less checks) in those methods where the actual dtype is unknown.

we are not using this, so its very confusing. I would just rather return an object dtyped array here (or have 2 methods).

we are not using this, so its very confusing

Non sequitur.

have 2 methods

... with almost perfect duplication of code, one of which perfectly generalizes the other, and without any performance gain?! I thought you liked to reduce code :-)

This method builds a 1d array of type dtype with given data. If it is impossible, it raises an error. This is pretty straightforward, and whenever np.array will have an ndmax parameter, all calls to this method will be replaced with a simple call to np.array(..., ndmax=1).

I guess I will explain again.

We are not using the dtype argument. So just eliminate it. I don't see utility in having it.

pep8speaks · 2017-12-14T14:05:05Z

Hello @toobaz! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 19, 2017 at 13:39 Hours UTC

jreback · 2017-12-15T01:04:29Z

asv_bench/benchmarks/ctors.py

@@ -33,3 +34,21 @@ def time_dtindex_from_series(self):

    def time_dtindex_from_index_with_series(self):
        Index(self.s)
+


lots of examples in the other asvs

jreback · 2017-12-15T01:05:18Z

pandas/core/dtypes/cast.py

+
+    Returns
+    -------
+    1-dimensional numpy array of dtype "dtype"


we are not using this, so its very confusing. I would just rather return an object dtyped array here (or have 2 methods).

jreback · 2017-12-15T01:06:17Z

pandas/tests/dtypes/test_cast.py

+        try:
+            problem = (dtype is not object and
+                       np.array(data, dtype=dtype).ndim != 1) and ValueError
+        except (ValueError, TypeError) as exc:


can you make 2 tests, 1 of which all succeed, 1 of which tests the ones that error

I can, but then I wouldn't parametrize (that would be a nightmare, since the cartesian product of parameters cannot be cleanly split). Let me try to add some comments to make it more clear instead.

jreback · 2017-12-15T23:44:44Z

asv_bench/benchmarks/ctors.py

+              [False, True]]
+
+    def setup(self, data_fmt, with_index):
+        N = 10**2


make this 10**4, this is too small

jreback · 2017-12-15T23:47:44Z

asv_bench/benchmarks/ctors.py

+    param_names = ["data_fmt", "with_index"]
+    params = [[lambda x: x,
+               list,
+               lambda arr: dict(zip(range(len(arr)), arr)),


add something with tuples here
add strings

jreback · 2017-12-15T23:48:19Z

asv_bench/benchmarks/frame_ctor.py

+
+
+class FromNDArray(object):
+


are there already list-like constructor asv's here? and from dict?

no idea (unrelated to this PR)

and the answer is?

no idea, as above

well since you are adding this one, should be very easy to .parametrize and add similar to the Series case, at least a few simple ones.

Sure, some simple ones are present in frame_ctor.py, above the code I moved from ctor.py. I can open an issue to remind adding more.

jreback · 2017-12-15T23:48:43Z

pandas/core/common.py

-            except ValueError:
-                # we have a list-of-list
-                result[:] = [tuple(x) for x in values]
+        # Avoid building an array of arrays:


is there an asv that hits this case?

I don't even think there is any valid code path that hits this case... which indeed should be suppressed in #18626

if there is not valid code, then let's remove it. or make a new issue. inside a PR doesn't help.

can you add the issue number here with a TODO

jreback · 2017-12-15T23:49:20Z

pandas/core/dtypes/cast.py

+
+    Returns
+    -------
+    1-dimensional numpy array of dtype "dtype"


I guess I will explain again.

We are not using the dtype argument. So just eliminate it. I don't see utility in having it.

jreback · 2017-12-15T23:51:00Z

pandas/tests/dtypes/test_cast.py

+    def test_cast_1d_array(self, dtype, datum1, datum2):
+        data = [datum1, datum2]
+        try:
+            # Conversion to 1d array is possible if requested dtype is object


please separate into 2 tests. This is so confusing as to what cases are what. If you need to skip a few tests on specific cases in the 'good' case, that is ok, but you need to clearly have a separate tests for the error cases. catching errors IN a test is a big no-no. it is impossible to tell what should succeed here.

toobaz · 2017-12-18T08:22:47Z

We are not using the dtype argument. So just eliminate it. I don't see utility in having it.

Yeah, but this is hardly an argument - you're not even seeing that the change you are firmly pretending in the code is in contrast with the change you are firmly pretending in the test.

But I guess I'll have to explain you in some other PR, now I don't have enough time.

An updated version of this one is on its way.

jreback

small comments.

jreback · 2017-12-18T12:51:35Z

asv_bench/benchmarks/ctors.py

+               list,
+               lambda arr: list(arr.astype(str)),
+               lambda arr: dict(zip(range(len(arr)), arr)),
+               lambda arr: [(i, -i) for i in arr],


some args are duplicated here

sorry, was reading the ( as [

jreback · 2017-12-18T12:52:52Z

asv_bench/benchmarks/frame_ctor.py

+
+
+class FromNDArray(object):
+


and the answer is?

jreback · 2017-12-18T12:53:16Z

pandas/core/common.py

-            except ValueError:
-                # we have a list-of-list
-                result[:] = [tuple(x) for x in values]
+        # Avoid building an array of arrays:


if there is not valid code, then let's remove it. or make a new issue. inside a PR doesn't help.

jreback · 2017-12-18T12:54:30Z

pandas/tests/dtypes/test_cast.py

+
+    @pytest.mark.parametrize('datum1', [1, 2., "3", (4, 5), [6, 7], None])
+    @pytest.mark.parametrize('datum2', [8, 9., "10", (11, 12), [13, 14], None])
+    def test_cast_1d_array(self, datum1, datum2):


are there fail cases? iow where this routine raises?

Not once you remove support for non-object dtypes (which is what you just asked me to do).

(well, as long as data has a len() and is iterable...)

right so can you add a test that fails for scalars then

jreback · 2017-12-18T12:54:49Z

pandas/tests/dtypes/test_cast.py

+    def test_cast_1d_array(self, datum1, datum2):
+        data = [datum1, datum2]
+        result = construct_1d_object_array_from_listlike(data)
+        # Direct comparison fails: https://github.com/numpy/numpy/issues/10218


jreback · 2017-12-18T14:03:40Z

asv_bench/benchmarks/ctors.py

+               list,
+               lambda arr: list(arr.astype(str)),
+               lambda arr: dict(zip(range(len(arr)), arr)),
+               lambda arr: [(i, -i) for i in arr],


sorry, was reading the ( as [

jreback · 2017-12-18T14:04:12Z

asv_bench/benchmarks/frame_ctor.py

+
+
+class FromNDArray(object):
+


well since you are adding this one, should be very easy to .parametrize and add similar to the Series case, at least a few simple ones.

jreback · 2017-12-18T14:04:58Z

pandas/core/common.py

-            except ValueError:
-                # we have a list-of-list
-                result[:] = [tuple(x) for x in values]
+        # Avoid building an array of arrays:


can you add the issue number here with a TODO

jreback · 2017-12-18T14:05:46Z

pandas/tests/dtypes/test_cast.py

+
+    @pytest.mark.parametrize('datum1', [1, 2., "3", (4, 5), [6, 7], None])
+    @pytest.mark.parametrize('datum2', [8, 9., "10", (11, 12), [13, 14], None])
+    def test_cast_1d_array(self, datum1, datum2):


right so can you add a test that fails for scalars then

jreback · 2017-12-18T14:06:57Z

pandas/core/dtypes/cast.py

+    Returns
+    -------
+    1-dimensional numpy array of dtype object
+    """


would not object to an
assert is_iterable(values) with a nice error message

The thing is: I can't think of any code path which could be hitting it. Scalar input to a Series() is (considered valid and) recasted to a 1-d before calling this. Similarly, an operation such as Series([1,2]) + 3 transforms 3 before hitting this. So I don't know what the error message could actually say.

not what i am asking

this is a completely internal
routine
it should fail with invalid input

it should fail with invalid input

Sure it does, TypeError: object of type 'int' has no len(). Which is pretty clear, considering the docstring, and precisely in light of the fact that this is an internal routine. That said, feel free to suggest an error message which is worth the cost of the additional assert.

add a raises section to the doc-string

codecov · 2017-12-18T18:17:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b32dd63). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #18769   +/-   ##
=========================================
  Coverage          ?   91.62%           
=========================================
  Files             ?      154           
  Lines             ?    51401           
  Branches          ?        0           
=========================================
  Hits              ?    47097           
  Misses            ?     4304           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.49% <100%> (?)`
#single	`40.84% <80%> (?)`

Impacted Files	Coverage Δ
pandas/core/common.py	`92.92% <100%> (ø)`
pandas/core/algorithms.py	`94.14% <100%> (ø)`
pandas/core/dtypes/cast.py	`88.6% <100%> (ø)`
pandas/core/ops.py	`91.77% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b32dd63...d0a6e48. Read the comment docs.

jreback

small comments. ping on green.

jreback · 2017-12-19T12:02:13Z

pandas/core/dtypes/cast.py

+    Returns
+    -------
+    1-dimensional numpy array of dtype object
+    """


add a raises section to the doc-string

…stlike

jreback · 2017-12-19T14:11:27Z

this will fail because of a conda issue, that I think is now fixed in master. I will merge after this is resolved (no need to repush).

jreback · 2017-12-19T14:41:08Z

thanks!

toobaz force-pushed the construct_1d_array_from_listlike branch from e72437c to dcf0136 Compare December 14, 2017 08:17

jorisvandenbossche reviewed Dec 14, 2017

View reviewed changes

jreback requested changes Dec 14, 2017

View reviewed changes

jreback added Dtype Conversions Unexpected or buggy dtype conversions Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Dec 14, 2017

toobaz force-pushed the construct_1d_array_from_listlike branch from dcf0136 to c88f05f Compare December 14, 2017 14:04

toobaz force-pushed the construct_1d_array_from_listlike branch from c88f05f to c7fbfb0 Compare December 14, 2017 14:27

jreback requested changes Dec 15, 2017

View reviewed changes

toobaz force-pushed the construct_1d_array_from_listlike branch 3 times, most recently from 61ebefb to 159427a Compare December 15, 2017 18:03

jreback requested changes Dec 15, 2017

View reviewed changes

toobaz force-pushed the construct_1d_array_from_listlike branch 2 times, most recently from 905046a to 8848344 Compare December 18, 2017 10:16

jreback requested changes Dec 18, 2017

View reviewed changes

This was referenced Dec 18, 2017

len-1 scalar is accepted as valid input for len >1 Series #18819

Closed

BUG: avoid unnecessary casting when unstacking index with unused levels #18460

Merged

jreback requested changes Dec 18, 2017

View reviewed changes

jreback reviewed Dec 18, 2017

View reviewed changes

toobaz force-pushed the construct_1d_array_from_listlike branch from 8848344 to e5b3cd5 Compare December 18, 2017 14:43

jreback requested changes Dec 19, 2017

View reviewed changes

toobaz mentioned this pull request Dec 19, 2017

asv tests for DataFrame constructor #18840

Closed

toobaz added 2 commits December 19, 2017 14:26

REF: implement and use construct_1d_array_from_listlike

d4d02a2

REF: Replace lib.list_to_object_array with construct_1d_array_from_li…

0afff94

…stlike

toobaz added 5 commits December 19, 2017 14:27

TST: asv benchmarks for Series constructors

f5f3773

REF: Solve circular dependency with python 2.7

44eedfa

TST: test_cast_1d_array

0892280

REF: only support object dtype

c4e635f

TST: Test invalid input to test_cast_1d_array_invalid_scalar

d0a6e48

toobaz force-pushed the construct_1d_array_from_listlike branch from e5b3cd5 to d0a6e48 Compare December 19, 2017 13:39

jreback added this to the 0.22.0 milestone Dec 19, 2017

jreback approved these changes Dec 19, 2017

View reviewed changes

jreback merged commit 04db779 into pandas-dev:master Dec 19, 2017

toobaz deleted the construct_1d_array_from_listlike branch December 19, 2017 15:02

		@@ -33,3 +34,21 @@ def time_dtindex_from_series(self):

		def time_dtindex_from_index_with_series(self):
		Index(self.s)



		class FromNDArray(object):



		class FromNDArray(object):



		class FromNDArray(object):

Construct 1d array from listlike #18769

Construct 1d array from listlike #18769

Conversation

toobaz commented Dec 13, 2017

toobaz commented Dec 14, 2017

codecov bot commented Dec 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz commented Dec 14, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Dec 14, 2017 • edited Loading

Comment last updated on December 19, 2017 at 13:39 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz commented Dec 18, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 18, 2017 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 19, 2017

jreback commented Dec 19, 2017

codecov bot commented Dec 14, 2017 •

edited

Loading

pep8speaks commented Dec 14, 2017 •

edited

Loading

toobaz Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 18, 2017 •

edited

Loading