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

GeoSegment is unnecessary #1165

Closed
rjyounes opened this issue Oct 7, 2024 · 4 comments · Fixed by #1180
Closed

GeoSegment is unnecessary #1165

rjyounes opened this issue Oct 7, 2024 · 4 comments · Fixed by #1180
Assignees
Labels
status: deferred to major release Involves a major change so deferred till next major release. An implementation may be specified.

Comments

@rjyounes
Copy link
Collaborator

rjyounes commented Oct 7, 2024

A GeoSegment is just the special case of a GeoRoute with two points. If it were to subclass GeoRoute, it makes the representation of a GeoRoute messy because it uses a different mechanism for ordering (comesFromPlace, goesToPlace rather than an ordered collection). The triples using those predicates will be inferred between every pair of consecutive points on the route.

Either we should keep the class and redefine it as a GeoRoute with two members, dropping the predicates from the definition, or deprecate/delete it.

@kstudzin
Copy link
Contributor

kstudzin commented Oct 7, 2024

We are correcting GeoRegion to GeoRoute in #1160.

I think the current definition, including the formal definition, leave it unclear if GeoSegment is a curve or a line. I had the impression it was a line, but looking at it a second time it wasn't 100% clear it can't be a curve.

For context, this came up due to the gist CCO mapping. CCO has a GeospatialLineString class, which is similar to GeoRoute and has a GeospatialLine subclass. I was trying to understand if GeoSegment and GeospatialLine were essentially equivalent

@rjyounes
Copy link
Collaborator Author

rjyounes commented Oct 8, 2024

GeoRoute and GeoSegment cannot contain curves, else the ordered collection of points or the comesFromPlace and goesToPlace values, respectively, would not be deterministic. They would each define, not a path, but an infinite set of paths.

@kstudzin
Copy link
Contributor

The way I think about it is that now we assume the function connecting the two points is a line. I was imagining overriding that with a more complex function. I think we should add the word "line" to the definition: "An ordered set of GeoPoints that defines a path of line segments from starting point to ending point."

@marksem
Copy link
Collaborator

marksem commented Jan 23, 2025

Dropping GeoSegment seems reasonable. GeoRoute seems sufficient for upper onto like gist. If someone finds a good justification for keeping GeoSegment, I agree it should be re-defined in terms of specialization of a GeoRoute.

@rjyounes rjyounes moved this from In Triage to To Do in gist Version 13.1.0 Jan 23, 2025
@rjyounes rjyounes added the status: deferred to major release Involves a major change so deferred till next major release. An implementation may be specified. label Jan 28, 2025
@rjyounes rjyounes moved this from In Triage to In Review in gist Version 13.1.0 Jan 28, 2025
@github-project-automation github-project-automation bot moved this from In Review to Done in gist Version 13.1.0 Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred to major release Involves a major change so deferred till next major release. An implementation may be specified.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants