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

Expose STAC temporal metadata #279

Merged
merged 11 commits into from
Jun 17, 2020

Conversation

Copy link
Contributor

@CloudNiner CloudNiner left a comment

Choose a reason for hiding this comment

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

A couple clarifying questions before a more thorough review to help me understand the changes made here

Copy link
Contributor

@CloudNiner CloudNiner left a comment

Choose a reason for hiding this comment

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

Unless there's a really compelling reason otherwise, we should exhaustive match our ADTs. Also should fix up the regression in parsing params noted previously.

I'm willing to let the ADT change go through, but I think it's a bad idea, see comment for a few potential alternative solutions.

@pomadchin
Copy link
Member Author

@CloudNiner the current OgcTime tree is the following:

sealed trait OgcTime
case object OgcTimeEmpty extends OgcTime
final case class OgcTimePositions(list: NonEmptyList[ZonedDateTime]) extends OgcTime
final case class OgcTimeInterval(start: ZonedDateTime, end: Option[ZonedDateTime], interval: Option[String]) extends OgcTime

And you think using OgcTimeInterval for everything would be a better idea?

  1. Talking into account that both WMS and WCS services can work with both
  2. Query evaluation in case of the OgcTimePositions works as the exact match, while OgcTimeInterval works as a search function on the time range, see link

@pomadchin
Copy link
Member Author

I'll work on the interfaces a bit more, but I think that OgcTimeInterval should represent an interval where the OgcTimePositions should represent a list of times that are can not be reprepsented as an interval. Using the List[OgcTimeInterval] sounds like a semantic issue. How would I know how to print such list in the WMS capabilities?

@pomadchin
Copy link
Member Author

As an alternative we can have a newtype List[OgcTimeInterval] which would have a configurable printer.

@CloudNiner
Copy link
Contributor

We chatted a bit about this after standup, adding some notes here.

I like the idea of making OgcTimeInterval explicitly an interval and seeing how that falls out, which would make end required in the case class, not sure if interval can stay an option or not. Maybe we can avoid the List[OgcTimeInterval] case for now and continue to just handle a single OgcTimeInterval? IIRC printing an OgcTimeInterval in the response depends on the spec for each of the response formats.

@pomadchin pomadchin force-pushed the feature/stac-temporal branch 3 times, most recently from 65f89a0 to 35e8655 Compare June 17, 2020 17:30
Copy link
Contributor

@CloudNiner CloudNiner left a comment

Choose a reason for hiding this comment

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

LGTM once the couple remaining minor issues are addressed and the build is green.

ogc/src/main/scala/geotrellis/server/ogc/OgcTime.scala Outdated Show resolved Hide resolved
ogc/src/main/scala/geotrellis/server/ogc/OgcTime.scala Outdated Show resolved Hide resolved

/**
* TODO: replace with source.extentIn(LatLng)
* see: https://github.com/locationtech/geotrellis/issues/3258
Copy link
Contributor

Choose a reason for hiding this comment

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

😭

@pomadchin pomadchin force-pushed the feature/stac-temporal branch 2 times, most recently from bd0a9b2 to 3932b75 Compare June 17, 2020 20:11
@pomadchin pomadchin merged commit 076e588 into geotrellis:develop Jun 17, 2020
@pomadchin pomadchin deleted the feature/stac-temporal branch June 17, 2020 21:41
@pomadchin pomadchin self-assigned this Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants