Fix new-style implementation of Series.append() method #262

kozlov-alexey · 2019-10-30T13:05:21Z

No description provided.

AlexanderKalistratov

@kozlov-alexey could you please write additional tests with nan, including cases when nan is in index?

hpat/datatypes/hpat_pandas_series_functions.py

hpat/hiframes/api.py

shssf · 2019-10-31T00:20:45Z

hpat/hiframes/api.py

+
+
+@overload(_append)
+def _append_overload(A, B):


I don't think this is a good place for hidden function used nowhere in hiframes.
Why do you need it overloaded?

@shssf I agree that there should be a better place for this function. Do you think it should be placed in a new file in datatypes?
The reason why it's needed is that without it I have to duplicate the same code in hpat_pandas_series_append, since both data and index can be numpy arrays/string_array and they are concatenated differently and also concatenation of array with list of arrays is not supported by Numba's np.concatenate, so it has to be implemented ourselves.

Moved to hpat/datatypes/common_functions.py

densmirn · 2019-10-31T05:46:04Z

hpat/datatypes/hpat_pandas_series_functions.py

+    self: :obj:`pandas.Series`
+           input series
+    to_append : :obj:`pandas.Series` object or :obj:`list` or :obj:`set`
+                Series (or list or tuple of Series) to append with self
+    ignore_index: :obj:`bool`, default False
+                If True, do not use the index labels.
+                Supported as literal value only
+    verify_integrity: :obj:`bool`, default False
+                If True, raise Exception on creating index with duplicates.
+                *unsupported*


Let's align indentations.

densmirn · 2019-10-31T05:54:26Z

hpat/datatypes/hpat_pandas_series_functions.py

+            def hpat_pandas_series_append_single_impl(self, to_append, ignore_index=False, verify_integrity=False):
+
+                new_data = hpat.hiframes.api._append(self._data, to_append._data)
+                new_index = hpat.hiframes.api._append(self.index, to_append.index)


Does it work with self.index? Shouldn't we use self._index?

@densmirn Yes, self.index returns np.arange index if self._index is None.
Thanks for the comment, I'll update my tests to check that series without index are handled too.

@densmirn Actually, it turned out tests with suffix index_default (e.g. test_series_append_single_index_default) check exactly that self.index is called, while self._index is None (as we unbox default series index into None).

densmirn · 2019-10-31T05:54:43Z

hpat/datatypes/hpat_pandas_series_functions.py

+            def hpat_pandas_series_append_list_impl(self, to_append, ignore_index=False, verify_integrity=False):
+
+                data_arrays_to_append = [series._data for series in to_append]
+                index_arrays_to_append = [series.index for series in to_append]


Does it work with series.index? Shouldn't we use series._index?

Yes, see above.

densmirn · 2019-10-31T05:55:15Z

hpat/datatypes/hpat_pandas_series_functions.py

-    def hpat_pandas_series_append_impl(self, to_append):
-        return pandas.Series(self._data + to_append._data)
+                new_data = hpat.hiframes.api._append(self._data, data_arrays_to_append)
+                new_index = hpat.hiframes.api._append(self.index, index_arrays_to_append)


The same, self.index -> self._index?

densmirn · 2019-10-31T07:08:36Z

hpat/tests/test_series.py

-        S2 = pd.Series([-2., 5.0])
-        # Test single series
-        np.testing.assert_array_equal(hpat_func(S1, S2), test_impl(S1, S2))
+        dtype_to_data = {'float': [[-2., 3., 9.1], [-2., 5.0]],


What do you think about using of globally defined input data for tests?

@densmirn Hmm, I would use that if there's a reason of testing wider range of values, but append doesn't do any calculations or doesn't depend on values being used in a series (at least it shouldn't). I prefer it this way, but if you really think there might be a problem with this approach, I can change it of course.

pep8speaks · 2019-11-05T17:36:47Z

Hello @kozlov-alexey! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-08 14:21:07 UTC

densmirn · 2019-11-05T17:55:11Z

hpat/datatypes/hpat_pandas_series_functions.py

+
+    if ignore_index_is_false:
+        def hpat_pandas_series_append_impl(self, to_append, ignore_index=False, verify_integrity=False):
+            if to_append_is_series == True:


You could add # noqa at the end of line to say linter skip this line.

@densmirn Thanks!

hpat/datatypes/common_functions.py

hpat/tests/test_series.py

shssf · 2019-11-06T14:12:18Z

hpat/datatypes/common_functions.py

+        return var == value
+
+
+def hpat_arrays_append(A, B):


Why this function is needed? As far as I understand you could use other decorator to register function to be used in JIT.

@shssf This should provide different overloads for the same append operation for various underlying data types (np.array, StringArray). What are you suggesting to use instead?

@shssf please specify the exact way to implement it, or let's close this discussion

shssf · 2019-11-06T14:13:06Z

hpat/datatypes/common_functions.py

+        return var.literal_value == value
+
+
+def has_python_value(var, value):


not easy to understand why with function by the name. Please add docstring.

shssf · 2019-11-06T14:13:09Z

hpat/datatypes/common_functions.py

+from hpat.str_arr_ext import (string_array_type, num_total_chars, append_string_array_to)
+
+
+def has_literal_value(var, value):


not easy to understand why with function by the name. Please add docstring.

shssf · 2019-11-08T14:22:55Z

hpat/hiframes/pd_series_ext.py

@@ -1288,7 +1288,7 @@ def hpat_pandas_series_ctor_impl(data=None, index=None, dtype=None, name=None, c

        '''' use binop here as otherwise Numba's dead branch pruning doesn't work
        TODO: replace with 'if not is_index_none' when resolved '''
-        if is_index_none == False:
+        if is_index_none == False:  # noqa


Fix it accordion PEP

shssf · 2019-11-08T14:23:44Z

hpat/str_arr_ext.py

+
+@numba.njit(no_cpython_wrapper=True)
+def append_string_array_to(result, pos, A):
+    # precondition: result is allocated with the size enough to contain A


Please add docstring

shssf · 2019-11-08T14:25:08Z

hpat/datatypes/hpat_pandas_series_functions.py

+
+    if ignore_index_is_false:
+        def hpat_pandas_series_append_impl(self, to_append, ignore_index=False, verify_integrity=False):
+            if to_append_is_series == True:  # noqa


fix style according PEP

shssf · 2019-11-08T14:26:40Z

hpat/datatypes/hpat_pandas_series_functions.py

+    else:
+        def hpat_pandas_series_append_ignore_index_impl(self, to_append, ignore_index=False, verify_integrity=False):
+
+            if to_append_is_series == True:  # noqa


fix style according PEP

@shssf It's not possible - with "if to_append_is_series" Numba will fail to compile, as dead branch pruning won't eliminate the dead branch (it's similar as in
https://github.com/kozlov-alexey/hpat/blob/45c06272f576ce53dc76038da08d59338770b39a/hpat/hiframes/pd_series_ext.py#L1278).

kozlov-alexey requested review from Hardcode84, shssf, AlexanderKalistratov and densmirn October 30, 2019 13:05

AlexanderKalistratov reviewed Oct 30, 2019

View reviewed changes

kozlov-alexey added the [WIP] Work in progress label Oct 30, 2019

shssf reviewed Oct 31, 2019

View reviewed changes

densmirn reviewed Oct 31, 2019

View reviewed changes

kozlov-alexey force-pushed the feature/refactor_series_append branch from 699f1fc to 03c9265 Compare November 5, 2019 17:36

densmirn approved these changes Nov 5, 2019

View reviewed changes

AlexanderKalistratov reviewed Nov 5, 2019

View reviewed changes

kozlov-alexey force-pushed the feature/refactor_series_append branch from 03c9265 to d1fb912 Compare November 6, 2019 10:21

kozlov-alexey added Ready for Review and removed [WIP] Work in progress labels Nov 6, 2019

kozlov-alexey requested a review from shssf November 6, 2019 11:25

shssf suggested changes Nov 6, 2019

View reviewed changes

shssf added Waiting on author and removed Ready for Review labels Nov 6, 2019

kozlov-alexey force-pushed the feature/refactor_series_append branch from d1fb912 to 50a7b52 Compare November 6, 2019 14:37

kozlov-alexey added Ready for Review and removed Waiting on author labels Nov 6, 2019

kozlov-alexey requested a review from shssf November 6, 2019 14:38

kozlov-alexey force-pushed the feature/refactor_series_append branch from 50a7b52 to f53c4db Compare November 6, 2019 16:31

kozlov-alexey added 2 commits November 7, 2019 12:52

Fix new-style implementation of Series.append() method

4822618

Fix for 'apending StringArrays drops NaNs problem' and minor changes

7579497

kozlov-alexey force-pushed the feature/refactor_series_append branch from f53c4db to 7579497 Compare November 7, 2019 10:14

kozlov-alexey requested a review from AlexanderKalistratov November 7, 2019 10:26

AlexanderKalistratov approved these changes Nov 7, 2019

View reviewed changes

Merge branch 'master' into feature/refactor_series_append

b6c5c36

shssf suggested changes Nov 8, 2019

View reviewed changes

shssf added Waiting on author and removed Ready for Review labels Nov 8, 2019

shssf merged commit b857c10 into IntelPython:master Nov 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix new-style implementation of Series.append() method #262

Fix new-style implementation of Series.append() method #262

kozlov-alexey commented Oct 30, 2019

AlexanderKalistratov left a comment

shssf Oct 31, 2019

kozlov-alexey Oct 31, 2019 •

edited

Loading

kozlov-alexey Nov 5, 2019

densmirn Oct 31, 2019

kozlov-alexey Nov 5, 2019

densmirn Oct 31, 2019

kozlov-alexey Oct 31, 2019

kozlov-alexey Nov 5, 2019 •

edited

Loading

densmirn Oct 31, 2019

kozlov-alexey Nov 5, 2019

densmirn Oct 31, 2019

densmirn Oct 31, 2019

kozlov-alexey Nov 5, 2019

pep8speaks commented Nov 5, 2019 •

edited

Loading

densmirn Nov 5, 2019

kozlov-alexey Nov 5, 2019

shssf Nov 6, 2019

kozlov-alexey Nov 6, 2019

AlexanderKalistratov Nov 7, 2019

shssf Nov 6, 2019

kozlov-alexey Nov 6, 2019

shssf Nov 6, 2019

kozlov-alexey Nov 6, 2019

shssf Nov 8, 2019

shssf Nov 8, 2019

shssf Nov 8, 2019

shssf Nov 8, 2019

kozlov-alexey Nov 8, 2019

		return var.literal_value == value


		def has_python_value(var, value):

		from hpat.str_arr_ext import (string_array_type, num_total_chars, append_string_array_to)


		def has_literal_value(var, value):

Fix new-style implementation of Series.append() method #262

Fix new-style implementation of Series.append() method #262

Conversation

kozlov-alexey commented Oct 30, 2019

AlexanderKalistratov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlov-alexey Oct 31, 2019 • 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

kozlov-alexey Nov 5, 2019 • 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

pep8speaks commented Nov 5, 2019 • edited Loading

Comment last updated at 2019-11-08 14:21:07 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

kozlov-alexey Oct 31, 2019 •

edited

Loading

kozlov-alexey Nov 5, 2019 •

edited

Loading

pep8speaks commented Nov 5, 2019 •

edited

Loading