Add setitem by Rubtsowa · Pull Request #394 · IntelPython/sdc

Rubtsowa · 2019-12-09T10:27:09Z

No description provided.

pep8speaks · 2019-12-11T07:09:32Z

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

In the file sdc/datatypes/hpat_pandas_series_functions.py:

Line 215:87: E502 the backslash is redundant between brackets

Comment last updated at 2019-12-27 07:02:08 UTC

…_setitem

densmirn · 2019-12-18T07:58:45Z

+        Parameters
+        ----------
+        series: :obj:`pandas.Series`
+               input series


Please fix indentation.

densmirn · 2019-12-18T08:01:39Z

        pd.testing.assert_series_equal(A1, A2)

    @skip_numba_jit
+    @skip_sdc_jit


Can we get the test passed instead of skipping this one?

densmirn · 2019-12-18T08:02:00Z

        self.assertEqual(hpat_func(S), test_impl(S))

    @skip_numba_jit
+    @skip_sdc_jit


The same. It's not good idea to skip the tests.

…_setitem

densmirn · 2019-12-18T11:47:43Z

+    # series[0] = numb
+    #
+    # series[2:6] = numb
+    #
+    # indices = pd.Series(np.asarray([1, 6, 7, 8, 9]))
+    # series[indices] = numb


I would propose to add 3 examples: for num index, for slice, for series index. Then insert all the examples to docstring.

Additionally let's initialize Series with 5 elements, I think 10 is too many, because the result takes much space in the docstring,

densmirn · 2019-12-18T11:49:47Z

-    if not isinstance(self, SeriesType):
-        raise TypingError('{} The object must be a pandas.series. Given: {}'.format(_func_name, self))
+
+    ty_checker = TypeChecker('Operator setitem().')


Is it correct to use brackets for setitem()?

This quite correctly. Operator getitem also uses.

Is there other reasons that it is correct?

densmirn · 2019-12-18T11:51:13Z

+
+        return hpat_pandas_series_setitem_idx_series_impl
+
+    raise TypingError('{} The index must be an Integer, Slice or a pandas.series. Given: {}'.format(_func_name, idx))


Moreover I would propose to use TypeCheker here.

densmirn · 2019-12-20T11:05:46Z

        with self.assertRaises(TypingError) as raises:
            hpat_func(S, ind2, value2)
-        msg = 'Operator setitem. The object idx' + '\n' + ' given: unicode_type' + '\n' + ' expected: int, Slice, Series'
+        msg = 'Operator setitem. The object idx' + '\n' + ' given: unicode_type' + '\n' \


Why not avoid concatenation of string via +?
msg = 'Operator setitem. The object idx\n given: unicode_type\n expected: int, Slice, Series'

densmirn · 2019-12-20T11:06:04Z

+
+        with self.assertRaises(TypingError) as raises:
+            hpat_func(S, ind1, value1)
+        msg = 'Operator setitem. The object value' + '\n' + ' given: unicode_type' + '\n' + ' expected: self'


Why not avoid concatenation of string via +?

densmirn · 2019-12-20T11:39:59Z

        with self.assertRaises(TypingError) as raises:
            hpat_func(S, ind1, value1)
-        msg = 'Operator setitem. The object value' + '\n' + ' given: unicode_type' + '\n' + ' expected: self'
+        msg = 'Operator setitem. The object value\n given: unicode_type\n expected: self'


Let's create message template for code reusage:

msg_tmpl = 'Operator setitem. The object {}\n given: unicode_type\n expected: {}' msg = msg_tmpl.format('value', 'self') # ... msg = msg_tmpl.format('idx', 'int, Slice, Series')

densmirn · 2019-12-20T11:41:33Z

+
+    if (isinstance(value, SeriesType) and not isinstance(self.dtype, value.dtype)) or \
+            not isinstance(self.dtype, type(value)):
+        ty_checker.raise_exc(value, 'self', 'value')


What do you mean 'self'? What does mean self expected?

…_setitem

kozlov-alexey · 2019-12-25T17:19:13Z

+    numb = 0
+    series = pd.Series(np.arange(5, 0, -1))  # Series of 5, 4, 3, 2, 1
+
+    series[0] = numb


I think it'd be better to rename 'numb' to 'value' (since the former is a word itself)

kozlov-alexey · 2019-12-25T17:21:45Z

+    *************************************************
+     Pandas Series operator :attr:`pandas.Series.set` implementation
+
+    Test: python -m sdc.runtests sdc.tests.test_series.TestSeries.test_series_setitem_unsupported


Please use a correct line to run the tests for setitem, i.e.
python -m sdc.runtests -k sdc.tests.test_series.TestSeries.test_series_setitem*

kozlov-alexey · 2019-12-25T17:22:17Z

+    Parameters
+    ----------
+    series: :obj:`pandas.Series`
+           input series


Please use the same indent

kozlov-alexey · 2019-12-25T17:37:55Z

+
+        return hpat_pandas_series_setitem_idx_series_impl
+
+    ty_checker.raise_exc(idx, 'int, Slice, Series', 'idx')


I think we should simply return None here, as this exception will not reach the end user anyway.

kozlov-alexey · 2019-12-25T17:46:22Z

        msg = 'Method pct_change(). The object periods'
        self.assertIn(msg, str(raises.exception))

+    def test_series_setitem_for_value(self):


It seems that all three positive tests only cover integer series case, so floats and strings are not covered.
It would be good to add them too, you can write an aggregated test for diff data types/values, like here:

sdc/sdc/tests/test_series.py

Line 2176 in abfa12b

for data in data_to_test:

…_setitem

kozlov-alexey

OK, let's merge this and deal with StringArray problems later.

…_setitem

kozlov-alexey · 2019-12-27T09:26:55Z

+            Test: python -m sdc.runtests sdc.tests.test_series.TestSeries.test_series_setitem_for_value
+            Test: python -m sdc.runtests sdc.tests.test_series.TestSeries.test_series_setitem_for_slice
+            """
+            self._data[idx] = value


OK, I think I now why it doesn't work for StringArrays - operator.setitem for StringArray expects the size of 'value' to be equal to the value of _data[idx]. Hence, you'll need special handling for string data.

Rubtsowa added 3 commits December 6, 2019 16:22

Impl operator setitem

c8e30d2

change

1b5abb9

Imp operator setitem for Series

9276344

Rubtsowa requested review from densmirn and shssf December 9, 2019 10:27

densmirn added Waiting on CI Waiting on author labels Dec 9, 2019

Rubtsowa added 2 commits December 11, 2019 09:35

comment old style

fe163d0

merge

6adaceb

Rubtsowa added 7 commits December 11, 2019 10:26

change

3ebef8b

change

aa06789

Add conditions in pd_series_ext for operator setitem

c427e84

merge

89c3ebc

change

f91346d

change

962af67

change

80cff79

densmirn requested a review from kozlov-alexey December 17, 2019 06:33

Rubtsowa added 3 commits December 17, 2019 09:36

fix code style issues

a352091

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

c61f858

…_setitem

change

9bbaffb

Rubtsowa added Coverage decreased Ready for Review and removed Waiting on CI Waiting on author labels Dec 18, 2019

densmirn reviewed Dec 18, 2019

View reviewed changes

Rubtsowa added 2 commits December 18, 2019 14:40

change and add example for setitem (now not work)

f203e91

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

845f70b

…_setitem

densmirn reviewed Dec 18, 2019

View reviewed changes

solution PEP 8 issues

3b11560

densmirn reviewed Dec 20, 2019

View reviewed changes

correctioned error message in test

420145a

densmirn reviewed Dec 20, 2019

View reviewed changes

add template for message error

196ec8d

densmirn approved these changes Dec 20, 2019

View reviewed changes

Rubtsowa requested a review from akharche December 25, 2019 08:06

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

dde8f52

…_setitem

Rubtsowa removed the request for review from shssf December 25, 2019 08:48

Rubtsowa added 2 commits December 25, 2019 12:12

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

7955114

…_setitem

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

abfa12b

…_setitem

kozlov-alexey reviewed Dec 25, 2019

View reviewed changes

Comment thread sdc/datatypes/hpat_pandas_series_functions.py

kozlov-alexey reviewed Dec 25, 2019

View reviewed changes

Rubtsowa added 4 commits December 26, 2019 11:02

correction examples, expansion tests

ecb0c42

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

b856813

…_setitem

correction tests

2ce7a57

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

96e3d40

…_setitem

kozlov-alexey approved these changes Dec 26, 2019

View reviewed changes

Rubtsowa added 2 commits December 26, 2019 17:34

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

76aede8

…_setitem

correction check

b74018f

kozlov-alexey approved these changes Dec 26, 2019

View reviewed changes

Merge branch 'master' of https://github.com/IntelPython/hpat into add…

fb3266e

…_setitem

kozlov-alexey reviewed Dec 27, 2019

View reviewed changes

kozlov-alexey approved these changes Dec 27, 2019

View reviewed changes

kozlov-alexey merged commit ab559d9 into IntelPython:master Dec 27, 2019

Rubtsowa deleted the add_setitem branch April 7, 2020 07:05


		return hpat_pandas_series_setitem_idx_series_impl

		raise TypingError('{} The index must be an Integer, Slice or a pandas.series. Given: {}'.format(_func_name, idx))


		return hpat_pandas_series_setitem_idx_series_impl

		ty_checker.raise_exc(idx, 'int, Slice, Series', 'idx')

Conversation

Rubtsowa commented Dec 9, 2019

Uh oh!

pep8speaks commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-12-27 07:02:08 UTC

Uh oh!

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kozlov-alexey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pep8speaks commented Dec 11, 2019 •

edited

Loading