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

Add required Timescale argument #10

Merged
merged 1 commit into from Sep 27, 2016
Merged

Conversation

timj
Copy link
Member

@timj timj commented Sep 26, 2016

No description provided.

@timj
Copy link
Member Author

timj commented Sep 26, 2016

This commit seems to have also been made directly to master in 8149c40

@@ -387,7 +387,7 @@ def getOutputId(self, expRefList, calibId):
outputId.update(calibId)
return outputId

def getMjd(self, dataId):
def getMjd(self, dataId, timeSystem=dafBase.DateTime.MJD):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a time scale (DateTime.UTC for the old behavior), not a time system. The default value is incorrect in this context (though it may do something because Python treats enums as small ints) and the variable name is misleading.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

Please fix the two errors pointed out by Tim and I before merging, or if already merged, please fix the merged code somehow.

@@ -396,7 +396,7 @@ def getMjd(self, dataId):
elif not dateObs.endswith("Z"):
dateObs += "Z"

return dafBase.DateTime(dateObs).get(dafBase.DateTime.MJD)
return dafBase.DateTime(dateObs, timeSystem).get(dafBase.DateTime.MJD)
Copy link
Contributor

Choose a reason for hiding this comment

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

The time scale for get defaults to TAI. Thus with the old behavior the ISO string would be treated as UTC, but the returned MJD would be TAI, introducing a 30-ish second offset. But that is not documented as the desired behavior. Please either document it or fix it by providing the time system argument to get so the method is self-consistent (UTC in, UTC out, or TAI in, TAI out).

N.b. was rebased to use C++ name, but vestiges may be visible on
master due to user error.
@@ -387,7 +387,7 @@ def getOutputId(self, expRefList, calibId):
outputId.update(calibId)
return outputId

def getMjd(self, dataId):
def getMjd(self, dataId, timescale=dafBase.DateTime.MJD):
Copy link
Member Author

Choose a reason for hiding this comment

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

@RobertLuptonTheGood this is still wrong. MJD is a DateSystem. The timescale you want is DateTime.UTC.

@@ -387,7 +387,7 @@ def getOutputId(self, expRefList, calibId):
outputId.update(calibId)
return outputId

def getMjd(self, dataId):
def getMjd(self, dataId, timescale=dafBase.DateTime.MJD):
"""Determine the Modified Julian Date (MJD) from a data identifier"""
Copy link
Member Author

Choose a reason for hiding this comment

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

You might want to indicate that this is returning the MJD TAI.

@ktlim ktlim deleted the tickets/DM-7742 branch August 25, 2018 06:50
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.

None yet

3 participants