PERF: Move Period class and related functions to Cython module
State: closed

I propose that Period should be a more basic type, like Timestamp.

I wanted to start with implementing frequencies with multipliers (see issue #7811), but the code for Period was all over the place.  So, I moved all code directly related to Period to a Cython module. Changed a bit of the logic to make calls from Cython instead of Python to get rid of some Python dependencies. However, the 'meat' of Period is still there. I will be refactoring Period to use Cython over the next week or two.

I've only done a bit of performance testing, but the results from moving Period are good so far without any real Cython refactoring.

Note: First commit in this stream is from #9415 because I needed that bug fixed to make this work.

Comments most welcome!


Labels:
name: Period
description: Period data type

Comments:
try not to change the meat of any tests (moving around is ok)
as much harder to see what actually changed

ideally we could merge this and then you can build enhancements on top in another pull request

ping when passing
you have a couple of unicode failures (returning bytes not unicode)

I didn't move any tests (and won't without discussion, and probably a separate PR unless writing new tests).

Very good. Can we get #9415 merged first or will you merge it as part of this?

@jreback Fixed. Some commits are different so please start fresh.

Can we hold off on that? I did not want to change any logic except to remove dependencies.

yep no prob on changing code - 

pls do a perf run (vbenches)

keep in mind it's not clear on how much of this code is well tested

so if u think something is not being hit addrl tests to validate are welcome 

That is, not until the next PR.

One comment, not on the actual content of this PR, but on the file structure: you created a new module `pandas.period`, but I don't think this should be exposed to the user? (as `pd.Period` or `pd.tseries.period` is still the entrypoint?) The top-level `pandas` namespace is already over-crowded I think)

I know for a lot of other cython/python modules this is the same (but I think we do this already to much). 

Possible options: make it `_period.pyx`, move it away from toplevel, or keep it in a separate file but 'include' it in `tslib.pyx` (but I am not very good acquainted with the structure of pandas folder regarding to cython/c files).

@jorisvandenbossche Right now, tslib is a monstrosity. I tried using `include "period.pyx"` at first but ran into more problems than just creating another module and using `import`.

If you put `period.pyx` in `pandas/src`, Cython overwrites `period.c` in the codebase. So, I put it in top level for the moment.

Honestly, I put off making that choice until comments. I was confused about what to do since there are other files to move or rename too.

@jorisvandenbossche I should also mentioned that names for namespaces of pandas Cython modules are defined in `setup.py` and the actual `pyx` file used for that name is defined there.

I think `pandas/src/_period.pyx` is fine for the file location. _Maybe_ name the module `pandas.tslib.period` if that can be done. If you agree, I'll try it.

pls run versus master to isolate these changes

I've been refactoring Period more today with success. Here's what I've got so far: 4a6c533c. I removed the tests for period_asfreq because it does not follow the behavior of Period.asfreq. Is that ok?

Let me know if I should push it to this PR or wait.

@jreback Gist comments don't give notifications. What are some good setting for vbench num calls and repeats? I'm running it on a desktop Intel i7, 16GB RAM, and mirrored RAID and not running anything intensive during vbench (like other tests).

Maybe I should do some performance testing between commits. At the very least the first in the stream.

ok merged #9415 

I just wanted you to be familiar with the perf testing....

try `-n 5` (it should just be somewhat stable), e.g. a rerun of this should be roughy the same.

you can use `-r 'period|timeseries'` to make the runs shorter (this is a regex to select on the name of the benchmarks'. note there are very few period specific benchmarks. adding more of these would be good.

Just letting you know about lack of gist notifications. I thought they were a feature of GitHub too. I read your gist comment after other discussion. No worries.

I'm going to try booting with no GUI to running vbench. I think it's getting in the way. I'm going to do some vbench tests on particular commits early and late in the stream to determine where my problem is.

vbench documentation and searching about vbench hasn't shown me much. Thanks for the extra input. And baring with me! :bowtie:

vbench docs are here: https://github.com/pydata/pandas/wiki/Performance-Testing

additions always welcome to this

Sorry, I meant that they shown me much about guidelines for number of calls and repeats parameters. I'll gladly add some guidelines about that if we can start an issue and get some consensus (after I examine that myself :smiley:).

I can't get conclusive results on my main workstation. I always have one or more inconsistent outliers. Going to try some other machines and get back on this soon.

Running the test on server grade hardware seems to reduce the variance. Also, my issue may have partly been due to using pyenv which compiles python by default with CFLAGS='-O3'.

ok, pls rebase and squash to a smaller number of commits.

pls let me know and we can get this in.

Ok I'll do that in a little bit. I can get the commit count down to three.

I have more work refactoring Period ready but will submit on another PR.

that's all fine

ping when revised and green

and add a release note referencing this PR number (you can put in performance section) 

@jreback can you review the name changes in the last commit? I used `pandas._period`, but I'm not sure if that's consistent with imports like `import pandas.algos as _algos`.

this all looks good. (e.g. see https://github.com/pydata/pandas/issues/5294) for how all things _should_ be named (e.g. like you are doing is good, these imports should be `_*`).

pls rebase on top of this: https://github.com/pydata/pandas/pull/9500

(I am going to merge soon).

As the setup.py is failing on windows after a clean (the path comparison was wrong).

With this (you will change `period.c` -> `period_helper.c` in `setup.py`), then it is good. and we can merge yours.

Two successive vbench runs before next rebase: https://gist.github.com/blbradley/cfb6e46f439dd1b2efd4

Only thing to show there is no likely performance regressions.

Good to go.

@blbradley looks good; except that `Period` needs to inherit from `PandasObject` (like the original).

I was trying to not subclass from `PandasObject` because it is in Python space. `Timestamp` does not inherit from `PandasObject` either.

@blbradley I know, and there are inconsitencies there as well.

Ok, on the list then is a MixIn in Cython that you _can_ sub-class from (like `StringMixIn`) to pride the string/unicode display things that always are forgotton.

Just make up a list of all of these issues, post it and then you can close via PRs.

merging this.