Skip to content
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

ENH: Initial implementation of calculation of astronomical Julian Date. #4041

Closed
wants to merge 5 commits into from
Closed

Conversation

timcera
Copy link
Contributor

@timcera timcera commented Jun 26, 2013

  • Added 'to_julian_date()' to Timestamp
  • Added 'to_julian_date()' to DatetimeIndex
  • Added tests for both methods.

Toying with the idea of using pandas for my astronomy and tidal analysis packages and I need the astronomical Julian Date. So here is an implementation and tests. I think I put these in the right place. Really wanted to have the same function for the Timestamp and DatetimeIndex so that both would benefit from improvements, but couldn't figure out how to make that work.

Kindest regards,
Tim

@jreback
Copy link
Contributor

jreback commented Jun 26, 2013

can you hook up travis (see contributing in main pandas dir)

def test_compare_1700(self):
r1 = 2342145.5
r2 = Timestamp('1700-06-23').to_julian_date()
assert(r1 == r2), "{0} is not equal to {1}".format(r1, r2)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you change this to: self.assertEqual(r1, r2)? or, even better:

self.assertEqual(r2, 2342145.5)

@jtratner
Copy link
Contributor

As an aside, you could probably combine most of your tests into 2 tests (check out some of the other test cases in tseries/tests and core/tests to get an example) - just nice to follow the format of the rest of the test suite.

@jreback
Copy link
Contributor

jreback commented Aug 23, 2013

@timcera how's this coming along? would be a nice addition....

@timcera
Copy link
Contributor Author

timcera commented Aug 23, 2013

This issue (#4065) is why I haven't worked on this. What is the use to me to implement a calendar that starts in 4714 BC (proleptic gregorian), if pandas can't represent the date?

I tried to wade through the pandas code to help out with #4065, but quickly got in way over my head.

Back to this PR, it seems that the only issues are that I need to hook up travis, and possibly change the tests? Is there anything else? It is possible that I could do these things soonish.

My main concern though that I haven't got any feedback on is whether this is the right way to implement this functionality. Should it be a part of the Timestamp and Datetimeindex OR should it be a seperate utility function? Is there a pandas way? What is the big picture in how this should be implemented? Should pandas even care about calendars other than gregorian? Would other calendars make sense? Should the julian date be a completely separate object with a 'to_gregorian()' function? Should I just keep this functionality in my code? Should it be represented as a tuple (integer day, float fraction of day) which would allow representation of smaller intervals. I don't have any tests for small intervals though I include nanosecond in the calculations - don't know whether it actually does anything.

Kindest regards,
Tim


class TestDateTimeIndex(unittest.TestCase):
def test_1700(self):
r1 = [2345897.5, 2345898.5, 2345899.5, 2345900.5, 2345901.5]
Copy link
Contributor

Choose a reason for hiding this comment

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

pls add an self.assert_(isinstance(r2,np.ndarray) just to confirm that it returns that type

@jreback
Copy link
Contributor

jreback commented Aug 23, 2013

@timcera

I think your implementation is just fine, and since they return float for Timestamp and an ndarray for DatetimeIndex I don't see a problem

The calendar issue is separate (and complicated), you might find this interesting (docs have a build problem, but you can get the idea)

@jreback
Copy link
Contributor

jreback commented Nov 3, 2013

need to remove the merge branches

you can just
git rebase -i < before the first commit >
and delete commits other than 76979a2

@timcera
Copy link
Contributor Author

timcera commented Nov 3, 2013

I have the rebase correct now, but trying to push to my repository with:

git push origin to_julian

Returns the following error:

To git@github.com:timcera/pandas.git
! [rejected] to_julian -> to_julian (non-fast-forward)
error: failed to push some refs to 'git@github.com:timcera/pandas.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Any suggestions?

@jtratner
Copy link
Contributor

jtratner commented Nov 3, 2013

use git push --force origin to_juilian

@jreback
Copy link
Contributor

jreback commented Feb 16, 2014

@timcera circling back on these open PR's....still interested in this?

pls rebase....#4065 was fixed...so let's see

@timcera
Copy link
Contributor Author

timcera commented Feb 17, 2014

Still interested.

Rebased to the latest. I think it looks right, but if there are any issues, would be glad to work at getting things in order.

Kindest regards,
Tim

@jreback
Copy link
Contributor

jreback commented Feb 17, 2014

converting a DatetimeIndex returns a float array yes?
I think it's reasonable to return a Float64index

pls add a release note under new features for 0.14 and a mention in v0.14.0.txt
as well as a doc string

ideally always would like to see your tests in tseries/test_timeseries.py
u can keep them in classes like u have now. (maybe need an expanded name to fit in though
eh TestDatetimeIndexJulsnDate )

@timcera
Copy link
Contributor Author

timcera commented Feb 23, 2014

Looking at this should probably include a Float64Index.to_gregorian(). Might actually be more useful than the 'Datetime.Index.to_julian()'. Might need to be clear in the name though - perhaps Float64.Index.julian_date_to_gregorian()?

 * Added 'to_julian_date()' to Timestamp
 * Added 'to_julian_date()' to DatetimeIndex
 * Added tests for both methods.
     Float64Index.
TST: Moved to_julian_date tests from
     pandas/tseries/tests/test_julian_date.py into
     pandas/tseries/tests/test_timeseries.py
DOC: Added mention of to_julian_date functionality
     to doc/source/release.rst and v0.14.0.txt
@timcera
Copy link
Contributor Author

timcera commented Feb 24, 2014

Travis was failing, but because of a testing bug fixed in a2c99fd

I rebased to the latest upstream and pushed that changes up again.

Kindest regards,
Tim

@timcera
Copy link
Contributor Author

timcera commented Feb 24, 2014

Forgot to add a docstring to Timestamp.to_julian()

I can squash these commits. I don't know what the preferred view is.

@jreback
Copy link
Contributor

jreback commented Feb 25, 2014

I think it makes sense to add to period / period index as well (esp since periods CAN represent dates that are out if bounds for time stamps )

but this will require a common mixin class for period index / DatetimeIndex (which I need to add anyhow)

so this looks ready to merge?

@timcera
Copy link
Contributor Author

timcera commented Feb 25, 2014

I think it is ready.

periods=5,
freq='D').to_julian_date()
self.assert_(isinstance(r2, Float64Index))
np.testing.assert_array_equal(r1, r2)
Copy link
Contributor

Choose a reason for hiding this comment

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

change these test comparisons to use tm.assert_index_equal(r1,r2)

@jreback
Copy link
Contributor

jreback commented Feb 26, 2014

merged via 10ad9dd

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants