Skip to content

Conversation

@alxmrs
Copy link
Contributor

@alxmrs alxmrs commented Feb 16, 2023

Fixes and improved codepath for parsing time from Tiffs. Useful for performing round-trip tests.

@alxmrs alxmrs requested review from deepgabani8, j9sh264 and mahrsee1997 and removed request for deepgabani8 February 16, 2023 22:44
@alxmrs alxmrs marked this pull request as ready for review February 16, 2023 23:40
Copy link
Collaborator

@mahrsee1997 mahrsee1997 left a comment

Choose a reason for hiding this comment

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

Overall looks good ... just a minor feedback point.

@alxmrs
Copy link
Contributor Author

alxmrs commented Mar 13, 2023

Thanks for the feedback. The changes are now updated.

Copy link
Collaborator

@mahrsee1997 mahrsee1997 left a comment

Choose a reason for hiding this comment

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

LGTM !

raise RuntimeError("Wrong regex passed in --forecast_time_regex.")
ds.attrs['start_time'] = start_time
ds.attrs['end_time'] = end_time
elif 'start_time' in ds.attrs and 'end_time' in ds.attrs:
Copy link
Contributor

Choose a reason for hiding this comment

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

3 points

  • Our tiff files do not preserve attrs so reading them with xr.open_dataset(file.tiff, engine='rasterio') will have empty ds.attrs.
  • 'start_time' and 'end_time' in dataset attrs is in 2021-01-01T00:00:00Z format so we need to parse it into datetime.datetime() for below datetime_value_s variable to initialize.
  • start_time is not being used. Can be skipped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good points Deep.
May be we can pick this #159 (comment) next & restructure _preprocess_tif altogether.

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.

4 participants