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

Add possibility to convert time during ascii and csv conversion #76

Merged
merged 4 commits into from Aug 15, 2017

Conversation

Projects
None yet
3 participants
@PierreZ
Copy link
Contributor

PierreZ commented Aug 2, 2017

With this fix, column 'TIME' can be converted into any astropy.Time supported subformat such as:

  • jd
  • mjd
  • decimalyear
  • unix
  • cxcsec
  • gps
  • plot_date
  • datetime
  • iso
  • isot
  • yday
  • fits
  • byear
  • jyear
  • byear_str
  • jyear_str

@PierreZ PierreZ force-pushed the PierreZ:time branch 2 times, most recently from 2226a03 to 493a6f9 Aug 2, 2017

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 2, 2017

Codecov Report

Merging #76 into master will increase coverage by 0.37%.
The diff coverage is 92.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   18.84%   19.22%   +0.37%     
==========================================
  Files          53       53              
  Lines        9466     9510      +44     
==========================================
+ Hits         1784     1828      +44     
  Misses       7682     7682
Impacted Files Coverage Δ
pyke/tests/test_kepconvert.py 100% <100%> (ø) ⬆️
pyke/kepconvert.py 22.6% <80%> (+4.16%) ⬆️
pyke/kepmsg.py 95.23% <0%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2563089...a6f0e88. Read the comment docs.

@barentsen

This comment has been minimized.

Copy link
Member

barentsen commented Aug 2, 2017

Thanks @PierreZ!! 👍

We are going to want to make it more clear in the docstring that these times are for "Solar System Barycenter" (cf. archive manual page 17). This is important because it is common to assume that e.g. an ISO timestamp is Earth-centered, but Kepler gives the time a photon would arrive in the Solar System Barycenter to take Earth's ever-changing position in the Solar System out of the equation.

I also wonder if we can add a specific unit-test to verify the conversion, e.g. we could check whether header keywords TSTART and DATE-OBS are equal if converted using the method used here.

(I'm being nitpicky because it's so easy to get time systems wrong.)

@PierreZ

This comment has been minimized.

Copy link
Contributor Author

PierreZ commented Aug 2, 2017

You're right to be nitpicky! I'll update the branch with a better docstring and another unit-test, is that ok for you?

@barentsen

This comment has been minimized.

Copy link
Member

barentsen commented Aug 2, 2017

@PierreZ I can't imagine anything that would make me happier =)

@barentsen

This comment has been minimized.

Copy link
Member

barentsen commented Aug 2, 2017

(Also: add yourself to AUTHORS.rst please!)

@mirca

mirca approved these changes Aug 3, 2017

Copy link
Member

mirca left a comment

Looks great to me! Thanks again @PierreZ

* byear_str
* jyear_str

This comment has been minimized.

@mirca

mirca Aug 3, 2017

Member

I guess you can remove the blank lines between items.


for conversion in SUPPORTED_CONVERSION:

This comment has been minimized.

@mirca

mirca Aug 3, 2017

Member

no need for this blank line here as well

start.format = conversion

assert start.value == dateobs.value

This comment has been minimized.

@mirca

mirca Aug 3, 2017

Member

nice test :) 👍

@PierreZ

This comment has been minimized.

Copy link
Contributor Author

PierreZ commented Aug 4, 2017

Unfortunately, I just left for vacation without my computer 😛 I will remove the blank lines on my return 👍

@mirca mirca requested a review from barentsen Aug 4, 2017

@mirca

This comment has been minimized.

Copy link
Member

mirca commented Aug 4, 2017

@PierreZ Nah, no worries =) if there is no major concern by @barentsen we can merge this and clean up later :)

@PierreZ

This comment has been minimized.

Copy link
Contributor Author

PierreZ commented Aug 6, 2017

As you wish 👍

@mirca mirca force-pushed the PierreZ:time branch from 6227ce8 to a2dc37c Aug 7, 2017

@@ -368,6 +425,9 @@ def kepconvert_main():
parser.add_argument('infile', help='Name of input file', type=str)
parser.add_argument('conversion', help='Type of data conversion', type=str,
choices=['fits2asc', 'fits2csv', 'asc2fits'], default='fits2asc')
parser.add_argument('timeformat', dest='timeformat', default='',
help="Export time into any subformat handled by astropy.Time",
type=str)

This comment has been minimized.

@barentsen

barentsen Aug 7, 2017

Member

We're going to want to make this an optional argument (i.e. change timeformat into --timeformat).

@@ -368,6 +409,9 @@ def kepconvert_main():
parser.add_argument('infile', help='Name of input file', type=str)
parser.add_argument('conversion', help='Type of data conversion', type=str,
choices=['fits2asc', 'fits2csv', 'asc2fits'], default='fits2asc')
parser.add_argument('--timeformat', dest='timeformat', default='',
help="Export time into any subformat handled by astropy.Time",

This comment has been minimized.

@barentsen

barentsen Aug 7, 2017

Member

I suggest adding a few examples to the help string, i.e.: ...any subformat handled by astropy.time (e.g. mjd, iso, decimalyear). It should also say default="jd".

@@ -121,12 +144,30 @@ def kepconvert(infile, conversion, columns, outfile=None, baddata=True,
for colname in tqdm(colnames):
try:
if colname.lower() == 'time':
work.append(table.field(colname) + bjdref)
if len(timeformat) > 0:

This comment has been minimized.

@barentsen

barentsen Aug 7, 2017

Member

For clarity, let's have timeformat="jd" as the default in the function definition, and then we can have if timeformat != "jd" here.

PierreZ and others added some commits Aug 2, 2017

@PierreZ PierreZ force-pushed the PierreZ:time branch from a2dc37c to a6f0e88 Aug 14, 2017

@PierreZ

This comment has been minimized.

Copy link
Contributor Author

PierreZ commented Aug 14, 2017

Back from vacation! Thanks @barentsen for the advice, just pushed a commit to apply your changes!

@mirca

This comment has been minimized.

Copy link
Member

mirca commented Aug 14, 2017

I guess this should be good to go? :) @barentsen

@barentsen

This comment has been minimized.

Copy link
Member

barentsen commented Aug 15, 2017

I think this is good to be merged! Thanks @PierreZ and @mirca! 👍

@mirca mirca merged commit a04788a into KeplerGO:master Aug 15, 2017

3 checks passed

codecov/patch 92.15% of diff hit (target 18.84%)
Details
codecov/project 19.22% (+0.37%) compared to 2563089
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreZ

This comment has been minimized.

Copy link
Contributor Author

PierreZ commented Aug 15, 2017

Thanks 👍

@PierreZ PierreZ deleted the PierreZ:time branch Aug 15, 2017

@mirca

This comment has been minimized.

Copy link
Member

mirca commented Aug 15, 2017

Thank you, @PierreZ =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment