Skip to content

ENH: support for nanosecond time in offset and period #2555

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.

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

Merged
merged 0 commits into from
Mar 14, 2013
Merged

ENH: support for nanosecond time in offset and period #2555

merged 0 commits into from
Mar 14, 2013

Conversation

wuan
Copy link
Contributor

@wuan wuan commented Dec 17, 2012

This is my attempt to fix the so far missing nanosecond support for pd.DatetimeIndex (see issue #1812).

The Python modification should be relatively easy to review, but unfortunately I had to do a bigger refactoring of period.c in order to support subsecond times.

Im still working on adding more tests and I will try to include the tests on C level as well (test_period.c, which can only be used standalone so far).

I'm looking forward to your remarks and I will be happy to modify the code to fit it into the project.

Cheers
Andreas

@wesm
Copy link
Member

wesm commented Jan 18, 2013

@cpcloud this is tragic guys. Major conflict with #2670 i think. can you guys have a look and let me know if it's possible to merge these pull requests without major sadness?

@cpcloud
Copy link
Member

cpcloud commented Jan 18, 2013

I get a build error with the nanosecond_time branch. This is probably because I'm building with bleeding-edge numpy, so I'll try it with the pip default and see if that works.

@ghost
Copy link

ghost commented Jan 18, 2013

What's the reason for requiring numpy >=1.7?

@cpcloud
Copy link
Member

cpcloud commented Jan 18, 2013

And why can't I build with bleeding edge numpy? The old_defines.h header should not be included with git master numpy. I'll check this out.

@wuan
Copy link
Contributor Author

wuan commented Jan 18, 2013

merging #2670 before should be possible. The most collisions are to be expected for period.c but they should be manageable. Only the "int64_t as i8" import introduces a lot of changes which may lead to conflicts.

@wuan
Copy link
Contributor Author

wuan commented Jan 18, 2013

The reason for requiring numpy >= 1.7 is the support for nanoseconds in np.datetime64 which was not there before 1.7

@wuan
Copy link
Contributor Author

wuan commented Jan 18, 2013

Maybe its a good idea to remove format only changes in both PRs to ease merging.

@wuan
Copy link
Contributor Author

wuan commented Jan 18, 2013

Branch did not build with bleeding edge numpy because of
#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION
in period.h. It should work now.

@ghost
Copy link

ghost commented Jan 29, 2013

Will this fail gracefully (or can be made to) if a user has numpy 1.6 installed?
Making numpy>=1.7 a hard dep might be a problem. I believe 1.6 is still, by far,
the common case.

@wuan
Copy link
Contributor Author

wuan commented Jan 29, 2013

You're right, for up to ms resolution numpy 1.6 should be enough. I'm not shure what it does at the moment, but I will try to make it as compatible as possible.

@ghost
Copy link

ghost commented Jan 29, 2013

Great, that would make it easier to get in.

I see you merged upstream into your branch (rather then rebasing), cpcloud can attest to
the pain and suffering this can create.
Would you like some git-fu help to squash things down and rebase on top of master?

@wuan
Copy link
Contributor Author

wuan commented Jan 29, 2013

Hope it is still possible to switch to rebasing. You are welcome!

@ghost
Copy link

ghost commented Jan 29, 2013

@wesm, is #2670 solid enough to use as base for this PR? If you could clear that in, I'll try
and help wuan through git-misery.

@ghost
Copy link

ghost commented Jan 29, 2013

@wuan, The last upstream merge you made was against a8abf1f.

try this:

git checkout nanosecond_time   
git checkout -b nanosecond_time_rebase # create identical branch and move to it
git reset a8abf1fdcc3d95306 # point HEAD at latest upstream merged

at this point, you're in the same state as if you were to start with a checkout of a8abf1f, and
then manually made all the changes to the files, without commiting anything.

$ git status
# On branch nanosecond_time_rebase
# Changes not staged for commit:
#   modified:   ci/install.sh
#   modified:   pandas/src/numpy_helper.h
#   modified:   pandas/src/period.c
#   modified:   pandas/src/period.h
#   modified:   pandas/src/util.pxd
#   modified:   pandas/tseries/converter.py
#   modified:   pandas/tseries/frequencies.py
#   modified:   pandas/tseries/offsets.py
#   modified:   pandas/tseries/period.py
#   modified:   pandas/tseries/tests/test_frequencies.py
#   modified:   pandas/tseries/tests/test_offsets.py
#   modified:   pandas/tseries/tests/test_period.py
#   modified:   pandas/tseries/tests/test_timeseries.py
#   modified:   pandas/tseries/tools.py
#   modified:   pandas/tslib.pyx
#   modified:   setup.py
#   modified:   test.sh
#
# Untracked files:

Now you need to break these up into individual commits as best you can.
code, tests, travis and setup can all go into seperate commits.

Of course, you can break things up even further - it's your code, you know best what makes sense.

git add --patch [optional filename]

will let you interactively choose chunks to add to the index, if you want to go that route.

Once that's done, you'll have a neat set of patches based on a8abf1f which
you can rebase to current master.

Following that, mitigating the 1.7 hard dependency is important, and it'll be much easier
to resolve conflicts against #2670.

@ghost
Copy link

ghost commented Mar 14, 2013

pushed to 0.12, still need to resolve numpy 1.7 dep issue .

@jreback jreback merged commit a8abf1f into pandas-dev:master Mar 14, 2013
@wuan
Copy link
Contributor Author

wuan commented Mar 14, 2013

Sorry, my changes were absent during your merge as I was just working on the transition to rebase.

@wuan
Copy link
Contributor Author

wuan commented Mar 14, 2013

Should I open a new pull request referencing this one, or is it possible to reopen the request?

My branch is now rebased against the pydata master.

Nanoseconds will not work in numpy < 1.7, so some test cases should be skipped in that case. This is still work in progress.

@wesm
Copy link
Member

wesm commented Mar 14, 2013

Wait did I miss something was this ready to merge?

@wuan
Copy link
Contributor Author

wuan commented Mar 14, 2013

As far as I remember we wanted to merge #2670 before. And at least the numpy 1.6 compatibility should be added.

@wesm
Copy link
Member

wesm commented Mar 14, 2013

@jreback don't make me take away your push rights =P

@jreback
Copy link
Contributor

jreback commented Mar 14, 2013

shoot, sorry...I didn't mean to merge this one....similary named to another by stephenwlin...

@wesm
Copy link
Member

wesm commented Mar 14, 2013

Can someone let me know if this is salvagable?

@jreback
Copy link
Contributor

jreback commented Mar 14, 2013

unless I am reading somewrong, nothing was actually merged here?

@wuan
Copy link
Contributor Author

wuan commented Mar 14, 2013

most of the changes were not there when the merge was done as I was transforming using rebase. I will have a closer look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants