Skip to content

Add TimeUnixPTP to time formats - #11143

Merged
taldcroft merged 1 commit into
astropy:masterfrom
maxnoe:unix_time_ptp
Dec 8, 2020
Merged

taldcroft merged 1 commit into
astropy:masterfrom
maxnoe:unix_time_ptp

Conversation

@maxnoe

@maxnoe maxnoe commented Dec 7, 2020

Copy link
Copy Markdown
Member

Description

This adds a new time format TimeUnixPTP that has epoch 1970-01-01T00:00:00 TAI as apposed to 1970-01-01T00:00:08 TAI of the TimeUnixTAI format.

This format is used by the Precision Time Protocol IEEE standard and by extension the White Rabbit hardware / software in high energy physics.

@github-actions github-actions Bot added the time label Dec 7, 2020
@maxnoe
maxnoe force-pushed the unix_time_ptp branch 3 times, most recently from 82812df to 7de730c Compare December 7, 2020 15:09

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks basically good @maxnoe, but I have a number of documentation suggestions.

Comment thread astropy/time/formats.py Outdated
Comment thread astropy/time/formats.py Outdated
Comment thread astropy/time/formats.py Outdated
Comment thread astropy/time/formats.py
Comment thread astropy/time/formats.py Outdated
Comment thread CHANGES.rst Outdated

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @maxnoe!

@taldcroft taldcroft added this to the v4.3 milestone Dec 8, 2020
@taldcroft
taldcroft merged commit ec98320 into astropy:master Dec 8, 2020
@maxnoe
maxnoe deleted the unix_time_ptp branch December 8, 2020 14:43
@taldcroft

Copy link
Copy Markdown
Member

@maxnoe - please see #11245. It turns out we miffed it with the original definition of unix_tai. The preferred plan now is to change unix_tai to be exactly what you implemented here for unix_ptp.

Are you OK with backing out this PR in favor of just using unix_tai?

@maxnoe

maxnoe commented Jan 14, 2021

Copy link
Copy Markdown
Member Author

I am against changing the defintion of an already released time format. That will really confuse a lot of people I think.

@taldcroft

Copy link
Copy Markdown
Member

Can you go over to #11245 and weigh in? The basic thought there is that unix_tai as defined was just wrong from the start and that likely nobody is really using it for that reason. (You are a perfect example.) But you have a very valid point.

@maxnoe

maxnoe commented Jan 14, 2021

Copy link
Copy Markdown
Member Author

Sure. If it turns out that there is no place where the current definition of unix_tai is used, I am all for just calling it a bug and fixing it, at which point unix_ptp is just unnecessary.

@taldcroft

Copy link
Copy Markdown
Member

It's not definitive, but there is not any application code on GitHub actually using unix_tai based searching GitHub for that term.

@maxnoe

maxnoe commented Jan 14, 2021

Copy link
Copy Markdown
Member Author

Ah, right. That's indeed weird.

taldcroft added a commit to taldcroft/astropy that referenced this pull request Jan 14, 2021
pllim added a commit that referenced this pull request Jan 15, 2021
Revert "Merge pull request #11143 from maxnoe/unix_time_ptp"
namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021
namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021
namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021
namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021
nstarman pushed a commit to nstarman/astropy that referenced this pull request Feb 28, 2021
nstarman pushed a commit to nstarman/astropy that referenced this pull request Feb 28, 2021
Akshat1Nar pushed a commit to Akshat1Nar/astropy that referenced this pull request Mar 9, 2021
Akshat1Nar pushed a commit to Akshat1Nar/astropy that referenced this pull request Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants