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

Update weather-mv's start_time, end_time parameters to string. #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deepgabani8
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

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

A nit to try to make the code simpler. Otherwise, this LGTM.

I have a slight hesitation that this will break some pipelines, though I realize it will make weather-mv compatible with others, still.

@@ -182,15 +182,15 @@ class AssetData:
name: The EE-safe name of the asset.
target_path: The location of the asset in GCS.
channel_names: A list of channel names in the asset.
start_time: Image start time in floating point seconds since epoch.
end_time: Image end time in floating point seconds since epoch.
start_time: Image start time in '%Y-%m-%dT%H:%M:%SZ' format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the same as isoformat? If it is, let's just call it that.

"""Turn a numpy datetime64 into UTC timestring."""
timestamp = float((np_time - np.datetime64(0, 's')) / np.timedelta64(1, 's'))
if isinstance(time, datetime.datetime):
return time.strftime('%Y-%m-%dT%H:%M:%SZ')
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is isofromat, let's use time.isoformat()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with the line below.

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.

2 participants