-
Notifications
You must be signed in to change notification settings - Fork 26
[uss_qualifier] rename and generalize VerticesResource to VolumeResource #1138
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
base: main
Are you sure you want to change the base?
Conversation
1a63d46
to
6165254
Compare
|
||
template: Volume4DTemplate | ||
|
||
def s2_vertices(self) -> list[s2sphere.LatLng]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation should apply to an outline; probably a Volume3D if we want to make it easier to support non-polygonal shapes like circles in the future. We don't want to put a method on a high-level object that operates only on a much narrower subcomponent because that reduces reusability for other things that use only the narrower subcomponent (e.g., if we later wanted S2 vertices from a Volume3D for some other reason, we would be likely to duplicate this logic since it's not obvious we should go looking in a much higher-level special-purpose VolumeSpecification for that existing code).
We can add convenience accessors to higher-level objects, but we should make sure the actual operation is always performed at the appropriate lower level. An accessor would probably look something like:
def s2_vertices(self) -> list[s2sphere.LatLng]:
return self.template.resolve({}).volume.s2_vertices()
with
class Volume3D(ImplicitDict):
def s2_vertices(self) -> list[s2sphere.LatLng]:
[the rough implementation shown here]
Plus, the implementation here isn't even correct because a Volume4DTemplate includes, for instance, transformations
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to using resolve()
and moved the logic for s2_vertices to Volume3D
fdedda1
to
e890826
Compare
e890826
to
6be9d17
Compare
@@ -33,12 +33,57 @@ The release notes should contain at least the following sections: | |||
|
|||
-------------------------------------------------------------------------------------------------------------------- | |||
|
|||
# Release Notes for v0.18.2 | |||
# Release Notes for v0.19.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped the minor number given that the change is breaking.
First step towards #1053:
Rename
VerticesResource
toVolumeSpecification
and let it have a single field of typeVolume4DTemplate
, which will allow us to use the optional (and dynamic) time-bounds out of the box.This PR does not yet update scenarios to make usage of the dynamic time-bounds, this will happen in a follow-up PR.