Skip to content

TYP #37715: Fix mypy errors in accessor.py #50005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

TYP #37715: Fix mypy errors in accessor.py #50005

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c677bc
Update accessor.py
bang128 Dec 2, 2022
7c1748a
Update accessor.py
bang128 Dec 7, 2022
12b7360
Update accessor.py
bang128 Dec 7, 2022
d929fe5
Update accessor.py
bang128 Dec 7, 2022
46556b2
Update accessor.py
bang128 Dec 7, 2022
01a287e
Update accessor.py
bang128 Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pandas/core/strings/accessor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,9 +569,7 @@ def cat(

# concatenate Series/Index with itself if no "others"
if others is None:
# error: Incompatible types in assignment (expression has type
# "ndarray", variable has type "Series")
data = ensure_object(data) # type: ignore[assignment]
data = Series(ensure_object(data))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary code-wise? Creating a Series adds unnecessary overhead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that was my first thought too but the other code branch does work with a series. @bang128 can you just run the asv benchmarks for cat and post the results to make sure no regression?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we have coverage here, would probably be better to use a specific example with timeit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it said that "expression has type "ndarray", variable has type "Series", I just converted the "ndarray" to "Series"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd What do you mean with run the asv benchmarks for cat?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd I apologize for my late response. It seems that I need conda to run asv benchmark, don't I? But I am facing a problem installing conda: even though I installed miniconda3, it still says that there is no conda in my machine. Actually, this is the first time I run asv benchmark, can you help me, please?

Copy link
Member

@WillAyd WillAyd Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is you installed conda but maybe didn't run conda init to have it get activated by default by your shell.

You can alternately try changing asv_bench/asv.conf.json to set the environment_type to virtualenv instead of conda if you can't get the conda setup working

Copy link
Contributor Author

@bang128 bang128 Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd Thank you for your recommendation. I ran asv benchmark successfully, and this is the result:

· Creating environments
· Discovering benchmarks
·· Uninstalling from virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter..
·· Installing 6c677bc <Fix-#37715-accessor.py> into virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter...
· Running 2 total benchmarks (2 commits * 1 environments * 1 benchmarks)
[ 0.00%] · For pandas commit bc987e7 (round 1/2):
[ 0.00%] ·· Building for virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter....
[ 0.00%] ·· Benchmarking virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter
[25.00%] ··· Running (strings.Cat.time_cat--).
[25.00%] · For pandas commit 6c677bc <Fix-#37715-accessor.py> (round 1/2):
[25.00%] ·· Building for virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter....
[25.00%] ·· Benchmarking virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter
[50.00%] ··· Running (strings.Cat.time_cat--).
[50.00%] · For pandas commit 6c677bc <Fix-#37715-accessor.py> (round 2/2):
[50.00%] ·· Benchmarking virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter
[75.00%] ··· strings.Cat.time_cat ok
[75.00%] ··· ============ ====== ======== ========= ============
other_cols sep na_rep na_frac
------------ ------ -------- --------- ------------
0 None None 0.0 14.1±2ms
0 None None 0.001 17.3±0.9ms
0 None None 0.15 17.5±0.6ms
0 None - 0.0 14.6±4ms
0 None - 0.001 8.90±0.4ms
0 None - 0.15 13.2±2ms
0 , None 0.0 15.4±0.7ms
0 , None 0.001 15.2±0.9ms
0 , None 0.15 17.7±1ms
0 , - 0.0 15.0±0.6ms
0 , - 0.001 9.58±0.3ms
0 , - 0.15 12.7±0.6ms
3 None None 0.0 44.5±0.8ms
3 None None 0.001 50.2±2ms
3 None None 0.15 55.0±3ms
3 None - 0.0 44.5±0.7ms
3 None - 0.001 47.8±0.7ms
3 None - 0.15 73.3±4ms
3 , None 0.0 70.7±2ms
3 , None 0.001 67.4±3ms
3 , None 0.15 58.7±4ms
3 , - 0.0 65.7±1ms
3 , - 0.001 65.0±2ms
3 , - 0.15 85.4±6ms
============ ====== ======== ========= ============

[75.00%] · For pandas commit bc987e7 (round 2/2):
[75.00%] ·· Building for virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter....
[75.00%] ·· Benchmarking virtualenv-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-scipy-sqlalchemy-tables-xlrd-xlsxwriter
[100.00%] ··· strings.Cat.time_cat ok
[100.00%] ··· ============ ====== ======== ========= =============
other_cols sep na_rep na_frac
------------ ------ -------- --------- -------------
0 None None 0.0 9.90±2ms
0 None None 0.001 9.58±1ms
0 None None 0.15 12.8±2ms
0 None - 0.0 8.07±0.6ms
0 None - 0.001 10.3±2ms
0 None - 0.15 11.7±0.7ms
0 , None 0.0 8.87±1ms
0 , None 0.001 8.74±0.08ms
0 , None 0.15 10.7±2ms
0 , - 0.0 7.54±0.5ms
0 , - 0.001 8.46±0.5ms
0 , - 0.15 10.7±0.7ms
3 None None 0.0 44.5±1ms
3 None None 0.001 47.2±1ms
3 None None 0.15 52.4±4ms
3 None - 0.0 43.1±3ms
3 None - 0.001 49.1±3ms
3 None - 0.15 71.6±6ms
3 , None 0.0 72.6±10ms
3 , None 0.001 71.7±4ms
3 , None 0.15 62.2±6ms
3 , - 0.0 68.2±1ms
3 , - 0.001 66.7±2ms
3 , - 0.15 81.5±5ms
============ ====== ======== ========= =============

   before           after         ratio
 [bc987e70]       [6c677bc2]
                  <Fix-#37715-accessor.py>

  •  7.54±0.5ms       15.0±0.6ms     1.99  strings.Cat.time_cat(0, ',', '-', 0.0)

  •  8.07±0.6ms         14.6±4ms     1.81  strings.Cat.time_cat(0, None, '-', 0.0)

  •    9.58±1ms       17.3±0.9ms     1.81  strings.Cat.time_cat(0, None, None, 0.001)

  •    8.87±1ms       15.4±0.7ms     1.74  strings.Cat.time_cat(0, ',', None, 0.0)

  • 8.74±0.08ms       15.2±0.9ms     1.74  strings.Cat.time_cat(0, ',', None, 0.001)

  •    10.7±2ms         17.7±1ms     1.65  strings.Cat.time_cat(0, ',', None, 0.15)

  •    9.90±2ms         14.1±2ms     1.42  strings.Cat.time_cat(0, None, None, 0.0)

  •    12.8±2ms       17.5±0.6ms     1.36  strings.Cat.time_cat(0, None, None, 0.15)

  •  10.7±0.7ms       12.7±0.6ms     1.19  strings.Cat.time_cat(0, ',', '-', 0.15)

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE DECREASED.

It seems a bad result, does it mean I need to change something?

Copy link
Member

@WillAyd WillAyd Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks for doing that! Yea looks like wrapping this in a Series slows things down - do you see any other way of getting the typing to work without the Series construction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try some other ways

na_mask = isna(data)
if na_rep is None and na_mask.any():
return sep.join(data[~na_mask])
Expand Down