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

Iso-duration #12

Merged
merged 9 commits into from
Aug 31, 2023
Merged

Iso-duration #12

merged 9 commits into from
Aug 31, 2023

Conversation

constantinius
Copy link
Collaborator

Proper ISO 8601 duration encoding for temporal dimension steps

Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Marking as draft until #10 is merged, as this includes the diff from that.

@gadomski gadomski marked this pull request as draft August 28, 2023 12:52
@constantinius constantinius marked this pull request as ready for review August 28, 2023 13:55
Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Is https://pypi.org/project/isoduration/ usable? If there's an off-the-shelf package that we can lean on, that'd be preferable to a re-implementation IMO.

I ran into the same issue in noaa-cdr: https://github.com/stactools-packages/noaa-cdr/blob/db4ebdc633a2cb1f27874b039edcbe761b81b214/src/stactools/noaa_cdr/time.py#L191-L215. There, I knew I had a limited subset of cases to handle, so I did re-implement, but since this is supposed to be a general-use library, we should avoid re-implementation if possible IMO.

@constantinius
Copy link
Collaborator Author

After some investigation I can say: unfortunately isoduration does not provide the arithmetic operations on their central Duration class that we need in this case.

The other option pandas.Timedelta is quite a heavy dependency and also does not cover all the bases.

For temporal dimensions we have a single conversion from timedelta to an ISO duration, which I hope is covered by the function. I would love to go ahead with this.

Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

iso_duration deserves some unit tests, I think, to ensure we're handling some common cases properly. At minimum:

  • One year
  • One month
  • One day
  • One week
  • One hour
  • One second
  • A duration that has everything
  • A duration that doesn't have any hours, minutes, or seconds

That will help future developers understand the expected behavior of the function.

Question: do we need a reverse function, one that parses a string into a timedelta?

src/stactools/datacube/stac.py Show resolved Hide resolved
src/stactools/datacube/stac.py Outdated Show resolved Hide resolved
src/stactools/datacube/stac.py Outdated Show resolved Hide resolved
src/stactools/datacube/stac.py Outdated Show resolved Hide resolved
@constantinius
Copy link
Collaborator Author

Question: do we need a reverse function, one that parses a string into a timedelta?

This has not yet come up. Currently we are building te timedeltas from netcdf/zarr metadata. They are usually expressed as somthing like "days since 1990-01-01"

Fixing results for various cases
Adding tests for iso_format
@constantinius
Copy link
Collaborator Author

I added a couple of unit-tests

@gadomski
Copy link
Collaborator

Appreciate the fixups, looks good!

@gadomski gadomski enabled auto-merge (squash) August 30, 2023 15:17
@gadomski gadomski self-requested a review August 30, 2023 22:10
Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

@constantinius this is not passing CI so needs fixes before it can go in and we release.

@gadomski gadomski merged commit d03ef1e into main Aug 31, 2023
4 checks passed
@gadomski gadomski deleted the iso-duration branch August 31, 2023 12:18
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