ASV: more for str.cat
State: closed

Planning to do a clean-up of the `Series.str.cat` internals and wanted to expand ASV-coverage before doing so.


Labels:
name: Benchmark
description: Performance (ASV) benchmarks

Comments:
Hello @h-vetinari! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`asv_bench/benchmarks/strings.py`](https://github.com/h-vetinari/pandas/blob/8d45108533549cbd28f6a450930551f89ef6cb41/asv_bench/benchmarks/strings.py) !


# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22652?src=pr&el=h1) Report
> Merging [#22652](https://codecov.io/gh/pandas-dev/pandas/pull/22652?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0976e1261781153603064eb67d66630a099edcd7?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22652/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22652?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22652   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files         169      169           
  Lines       50708    50708           
=======================================
  Hits        46740    46740           
  Misses       3968     3968
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.58% <ø> (ø)` | :arrow_up: |
| #single | `42.35% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22652?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22652?src=pr&el=footer). Last update [0976e12...67503c6](https://codecov.io/gh/pandas-dev/pandas/pull/22652?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@TomAugspurger 

ping because you managed the CirlceCI transition from 1.0 to 2.0. It seems that new setup does not respect `[ci skip]` - I view the CI as a limited resource (appveyor recently fluctuating up to something like 12h waiting time), so `[ci skip]` can be useful for avoiding a CI run where it's pointless (like this PR, which changes nothing in code/docs; and PEP8 runs anyway).
Last I saw CircleCI had an issue open for ci-skip. Not sure what the status
is.

On Sun, Sep 9, 2018 at 4:33 PM h-vetinari <notifications@github.com> wrote:

> @TomAugspurger <https://github.com/TomAugspurger>
>
> ping because you managed the CirlceCI transition from 1.0 to 2.0. It seems
> that new setup does not respect [ci skip] - I view the CI as a limited
> resource (appveyor recently fluctuating up to something like 12h waiting
> time), so [ci skip] can be useful for avoiding a CI run where it's
> pointless (like this PR, which changes nothing in code/docs; and PEP8 runs
> anyway).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22652#issuecomment-419746527>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHImB2sLu_oynoGv8HnqLkxQhR88kEks5uZYkRgaJpZM4Wgcb6>
> .
>

@TomAugspurger 

not sure where you saw it, but https://discuss.circleci.com/search?q=ci%20skip is pretty messy with lots of duplicates (due to their auto-closing policy for issues after a while with no activity). One of the closer ones seems to be https://discuss.circleci.com/t/ci-skip-does-not-work-in-pull-requests/5308

The 2.0 docs (https://circleci.com/docs/2.0/skip-build/) mention that
> By default, CircleCI automatically builds a project whenever you push changes to a version control system (VCS). You can override this behavior by adding a [ci skip] or [skip ci] tag anywhere in a commit’s title or description.
[...]
**Note**: This feature is not supported for fork PRs.

which is weird, because it *did* work for 1.0 in this repo (side note: the docs for that don't exist anymore: https://circleci.com/docs/1.0/skip-a-build/).

On an unrelated note: would you care to comment about the PR itself? :)
https://discuss.circleci.com/t/ci-skip-does-not-work-in-pull-requests/5308/10 is the one I saw.

Agreed it's unfortunate. If a maintainer is around you can ask them to cancel it :/

No strong thoughts on how the PR, other than a consideration of runtime. It seems like these should run pretty quickly.
@TomAugspurger 
> Agreed it's unfortunate. If a maintainer is around you can ask them to cancel it :/

Maintainer time is an even more precious resource than CI time, so I think I won't bother you guys. ;-)

> No strong thoughts on how the PR, other than a consideration of runtime. It seems like these should run pretty quickly.

Here's the output of an ASV run against master. Of course, `N` (and the number of columns for `others`) could be tuned as desired, but I don't think it's so bad (and due to decrease a fair amount - hopefully - by an upcoming PR).

```
       before           after         ratio
     [0976e126]       [2e70311e]
            594ms            609ms     1.03  strings.Cat.time_cat(5, ',', '-', 0.0)
            594ms            609ms     1.03  strings.Cat.time_cat(5, ',', '-', 0.0001)
            594ms            594ms     1.00  strings.Cat.time_cat(5, ',', '-', 0.1)
            375ms            375ms     1.00  strings.Cat.time_cat(5, ',', None, 0.0)
            359ms            344ms     0.96  strings.Cat.time_cat(5, ',', None, 0.0001)
            297ms            312ms     1.05  strings.Cat.time_cat(5, ',', None, 0.1)
           failed            453ms      n/a  strings.Cat.time_cat(5, None, '-', 0.0)
            438ms            438ms     1.00  strings.Cat.time_cat(5, None, '-', 0.0001)
            453ms            453ms     1.00  strings.Cat.time_cat(5, None, '-', 0.1)
            344ms            359ms     1.05  strings.Cat.time_cat(5, None, None, 0.0)
            344ms            344ms     1.00  strings.Cat.time_cat(5, None, None, 0.0001)
           failed            297ms      n/a  strings.Cat.time_cat(5, None, None, 0.1)
         46.9±0ms       46.9±0.7ms     1.00  strings.Cat.time_cat(None, ',', '-', 0.0)
         54.7±4ms       46.9±0.7ms    ~0.86  strings.Cat.time_cat(None, ',', '-', 0.0001)
         54.7±3ms         62.5±4ms    ~1.14  strings.Cat.time_cat(None, ',', '-', 0.1)
         44.3±3ms         44.3±3ms     1.00  strings.Cat.time_cat(None, ',', None, 0.0)
         62.5±4ms         62.5±4ms     1.00  strings.Cat.time_cat(None, ',', None, 0.0001)
         70.3±1ms         78.1±1ms    ~1.11  strings.Cat.time_cat(None, ',', None, 0.1)
           failed       41.7±0.7ms      n/a  strings.Cat.time_cat(None, None, '-', 0.0)
       41.7±0.7ms         46.9±4ms    ~1.12  strings.Cat.time_cat(None, None, '-', 0.0001)
         54.7±0ms         54.7±0ms     1.00  strings.Cat.time_cat(None, None, '-', 0.1)
           failed         62.5±1ms      n/a  strings.Cat.time_cat(None, None, None, 0.0)
           failed         58.6±5ms      n/a  strings.Cat.time_cat(None, None, None, 0.0001)
           failed         54.7±0ms      n/a  strings.Cat.time_cat(None, None, None, 0.1)
```
@WillAyd 
FWIW, @TomAugspurger said above:
> No strong thoughts on how the PR, other than a consideration of runtime. It seems like these should run pretty quickly.
Thanks!