JuliaGeo / GeoInterfaceRFC.jl

MIT License
4 stars 0 forks source link

Nice interface! #6

Closed juliohm closed 4 years ago

juliohm commented 4 years ago

I love this well-structured and organized source code :100: I was able to digest it in a few minutes without going back and forth in different files. Thank you for this amazing effort.

I have a few questions:

  1. What is the pointonsurface trait?
  2. Can we generalize area to volume and reinterpret it as an N-dimensional trait? Like in the volume of a 2D object is its area? Would it make sense to erase area from the traits list and only use volume so that user code is agnostic to dimension?
  3. Similar question for the pointonsurface, is there anything specific to 2D or it could be generalized?
  4. What is the "m" coordinate in the defaults.jl?

Please let me know if you would like help with docstrings, I saw that some functions don't have documentation.

juliohm commented 4 years ago

Another set of questions:

  1. I like the idea of ncoord as opposed to ndims to avoid conflict with Base names. Did you remove the "s" at the end for some English reason? I am not a native speaker, and so for me it sounds more natural to write ncoords and npoints as opposed to the current ncoord and npoint. Both are good options, I am just curious if my English is off in other cases.
  2. Do we have a trait to recover the coordinate machine type? Something like coordtype returning Float64, Float32 ...
juliohm commented 4 years ago
  1. What do you think of renaming centroid to center? One could argue that the differentiation between centroid and center is obsolete and that center would be enough to translate the concept of a point in the "middle" of the shape.
yeesian commented 4 years ago

Thanks for the review! The names area, centroid, and pointonsurface all came from https://www.ogc.org/standards/sfa, so my preference is to not change the names unless there's a different specification we want to target.

You're right about the plural form: it isn't for english reasons that the s at the back is removed. But I can also see it being argued the other way, where "dim" is itself a truncation of the word "dimensions" (rather than "dimension"). See https://numpy.org/doc/stable/reference/generated/numpy.ndarray.ndim.html as a precedent.

juliohm commented 4 years ago

Thank you @yeesian for the helpful link. I couldn't find the pointonsurface in the document (the PDF is not searchable), is there a short definition that you can share?

About the area , perhaps we can add some higher-level function volume that dispatches to area in 2D geometries?

I think it is fine to use centroid, it just feels a bit old. Perhaps we can introduce an alias const center = centroid?

Regarding the s at the end of npoints and ncoords, any chance we can adopt it here? I am currently using ncoords and npoints in GeoStatsBase.jl so it would be nice to just inherit them from GeoInterface.jl. Functions in Base also tend to add the s at the end like ndims so we could follow the pattern.

yeesian commented 4 years ago

I couldn't find the pointonsurface in the document (the PDF is not searchable), is there a short definition that you can share?

It's in section 6.1.10.1, 6.1.10.2, 6.1.13.1, 6.1.13.2, A.3.3.6 of the PDF. The definition is that it'll return an (arbitrary) point on this surface.

Regarding the s at the end of npoints and ncoords, any chance we can adopt it here?

I think an effort at integrating GeoInterfaceRFC.jl and GeoStatsBase.jl will be great! Feel free to put out a proposal and plan for the corresponding changes that will need to be made (be it in this issue, or in a gist) for volunteers to assess the overall effort and discuss the changes required. Given that GeoInterfaceRFC has already been implemented for a few other packages, my recommendation is for the changes to be made to GeoStatsBase.jl rather than vice-versa to reduce overall coordination/work.

juliohm commented 4 years ago

@evetion can you also share your thoughts on these proposals? I'd be happy to submit a PR with the changes as suggested by @yeesian.

The definition is that it'll return an (arbitrary) point on this surface.

Can we also generalize this to a point on the geometry as opposed to a 2D surface? I feel that these standards for features are heavily inspired by 2D applications in maps, but nothing inhibits us from generalizing the interface for N-dimensional spaces. It can be a good contribution.

yeesian commented 4 years ago

Can we also generalize this to a point on the geometry as opposed to a 2D surface? I feel that these standards for features are heavily inspired by 2D applications in maps, but nothing inhibits us from generalizing the interface for N-dimensional spaces.

To my understanding, the SF architecture is heavily influenced by previous studies in e.g. http://www.dpi.inpe.br/gilberto/references/clementini_topological_relations.pdf, and generalizing it to N-dimensional spaces is a non-goal for this RFC because there is already a lot of work to be done for complex polygons and getting GDAL/etc (drivers for various geospatial file formats) working with a unifying interface that can be used with GeometryBasic.

In my view, supporting arbitrary N-dimensional space sounds theoretically nice but has little incremental value for the vast majority of applications. Given the long history of abandoned attempts, I'd caution against any form of scope-creep until the RFC (in its current form) has formally landed.

juliohm commented 4 years ago

generalizing it to N-dimensional spaces is a non-goal for this RFC because there is already a lot of work to be done for complex polygons and getting GDAL/etc (drivers for various geospatial file formats) working with a unifying interface that can be used with GeometryBasic.

How this goal inhibits us from pursuing a more N-dimensional interface? Wouldn't it actually save us from having to revisit this at a later point where the interface can't be changed anymore?

In my view, supporting arbitrary N-dimensional space sounds theoretically nice but has little incremental value for the vast majority of applications.

It can really simplify code that consumes the interface. For instance, it is quite annoying to branch things manually with if-else in the code as opposed to just let multiple-dispatch call the appropriate volume on the geometry. A single verb in this case volume is better than many verbs volume, area, length all representing a measure in a measure theory sense.

juliohm commented 4 years ago

http://www.dpi.inpe.br/gilberto/references/clementini_topological_relations.pdf

This document starts a discussion where the dimension is only defined up to 2. This is really an issue for me because I'd like to apply geostatistical methods to 3D domains as well. We shouldn't target an interface that has this built-in assumption because of past design decisions. These standards out there help us shape the interface we are after, but shouldn't constrain us.

yeesian commented 4 years ago

This is really an issue for me because I'd like to apply geostatistical methods to 3D domains as well. We shouldn't target an interface that has this built-in assumption because of past design decisions. These standards out there help us shape the interface we are after, but shouldn't constrain us.

Please see https://github.com/JuliaGeo/GeoInterfaceRFC.jl/blob/master/PROPOSAL.md#q4-i-really-do-not-like-the-choice-of-names-in-this-package-ill-also-like-to-be-able-to-export-some-of-the-functions-for-ease-of-use-what-should-i-do. There's nothing barring a next (major/breaking) version of GeoInterface to come after this one, but I want to make it very clear that bikeshedding conversations like these are really draining because they require developers to continually re-question scoping assumptions without ever having a clear end-point for its completion.

I do not want the limited support for 3D geometries from blocking/derailing the development and release of this version of the RFC which has been many months in the making. I had spent months/years participating in design conversations and experimenting across different packages/drivers to settle on https://www.ogc.org/standards/sfa as the appropriate scope for an attempt at improving on the current GeoInterface.jl across Shapefile/GeoJSON/GDAL/GEOS/etc with the volunteers that we have today.

juliohm commented 4 years ago

Maybe I am misinterpreting something, but which changes I proposed above would be problematic regarding this 2D->3D scope? I can't see how changing names like area to volume and pointonsurface to pointongeom today would inhibit us from releasing the interface tomorrow. Is there any additional design issue that is a consequence of the renaming suggested?

yeesian commented 4 years ago

This is really an issue for me because I'd like to apply geostatistical methods to 3D domains as well. [...] which changes I proposed above would be problematic regarding this 2D->3D scope?

It's straining my belief that renaming two methods will make you better able to apply "geostatistical methods to 3D domains" using the rest of the interface in its current form.

The representation of vector geometries as nested vectors of points does not generalize well to higher dimensions, so renaming things so that they make sense for N-dimensions is a weak argument. It's not clear to me that volume(country) is a strict improvement on area(country) for it to be worth the deviation from the choice of names in the SFA.

These standards out there help us shape the interface we are after, but shouldn't constrain us. [...] I can't see how changing names like area to volume and pointonsurface to pointongeom today would inhibit us from releasing the interface tomorrow. Is there any additional design issue that is a consequence of the renaming suggested?

In my opinion, the lack of constraints in the past has hindered us more than it has helped us, and the renaming suggestions deviate from the policy (in the README) of aligning to https://www.ogc.org/standards/sfa.

juliohm commented 4 years ago

It's straining my belief that renaming two methods will make you better able to apply "geostatistical methods to 3D domains" using the rest of the interface in its current form.

You are suggesting that we should write code in a consumer package that reads like:

volume(g) = # my own implementation of volume for 3D
algorithm(g) = ncoord(g) == 2 ? area(g) : volume(g)

and then repeat this pattern for any function name that is 2D? I asked @visr in another issue if the interface proposed here was limited to 2D and he said that the effort was not limited to 2D. Your comments, however, suggest that 3D is a secondary concern.

The representation of vector geometries as nested vectors of points does not generalize well to higher dimensions, so renaming things so that they make sense for N-dimensions is a weak argument.

Can you elaborate on this statement? Perhaps there is some assumption that I am not aware of. I don't have much background in the SFA standard, I just feel that fixing a dimension to 2 prematurely is an issue.

It's not clear to me that volume(country) is a strict improvement on area(country)

That is why I thought that adding a default implementation as volume(g) = ncoord(g) == 2 ? area(g) : @error "not implemented" and making it part of the interface could save consumers of this same interface from rewriting their own volume, which would not be considered "official".

and the renaming suggestions deviate from the policy (in the README) of aligning to https://www.ogc.org/standards/sfa

If there is anything about the standard that is 2D-specific, then please let me know, it would make things clearer moving forward.

yeesian commented 4 years ago

Most of the questions have been answered, and this is evolving more into a feature request (if not a new RFC), which will not get resolved in the setting of a single Github issue thread, so I'm closing this issue.

I also saw the original thread in https://github.com/JuliaEarth/GeoStats.jl/issues/104 and have commented there.

juliohm commented 4 years ago

So where it will be solved then if not in an issue? I don't know why the discussion needs to be closed when other people didn't have a chance to share their opinions. In particular, I was waiting for @evetion and @visr 's take on this given that they are aware of many of my concerns with 2D only geometric specs.

yeesian commented 4 years ago

I asked @visr in another issue if the interface proposed here was limited to 2D and he said that the effort was not limited to 2D.

My interpretation of https://github.com/JuliaEarth/GeoStats.jl/issues/104#issuecomment-677453020 is that GeometryBasics is not limited to 2D.

There is nothing inherent about the goals of GeoInterface for it to be restricted to 2D.

If you want to be using geometrical primitives for GeoStats, the easier path is to use GeometryBasics rather than this interface.

yeesian commented 4 years ago

This repository/package has the SFA spelt out in the README as its scope. Anything outside of it is a non-goal so, following https://opensource.guide/best-practices/, I'm learning to say no here.

It's okay to begin with an issue called "Nice interface!" that evolves into a discussion about what the future-looking state of GeoInterface might look like. The original intention of this issue has become clear at this point: you have a feature request for 3D geometries in a manner that doesn't fall within the scope of the current RFC. That is an absolutely fair request, but it is open-ended in scope and it will be more considerate to have it in settings like discourse (see e.g. https://blog.atom.io/2016/04/19/managing-the-deluge-of-atom-issues.html).

This issue is closed, but the conversation/thread is not locked so people can still comment.

juliohm commented 4 years ago

I think it all comes down to a possible misinterpretation of @visr 's comment regarding the 2D property of the interface. In any case, I will see to what extent I can reuse or interop with the interface proposed here from the GeoStats.jl side. If I am not able to pursue the goals I have in mind, I can come back and propose an update to the spec, or develop a separate interface for N-D spaces.

yeesian commented 4 years ago

Thank you! I really appreciate it

visr commented 4 years ago

@juliohm sorry for not being more specific in my comment regarding 2D/3D here: https://github.com/JuliaEarth/GeoStats.jl/issues/104#issuecomment-677453020. I was talking about GeometryBasics. GeoInterface / SFA allows coordinates to hold a third dimension, hence ncoord, but otherwise takes a typical 2D view on the geometries and operations, as if viewed from infinite height.

  1. What is the "m" coordinate in the defaults.jl?

Here is a pretty good explanation: https://www.ibm.com/support/pages/what-are-semantics-m-coordinate-measure

  1. Do we have a trait to recover the coordinate machine type? Something like coordtype returning Float64, Float32 ...

Not right now, no. SFA says it should be Float64, and apart from GeometryBasics I think the other geometries such as GEOS/PROJ all are fixed to Float64.

evetion commented 4 years ago

Let me know if there are any open questions left, I think all of them are answered.

My two cents, in any case, SFA is indeed quite limited and we've already tried to expand the scope a bit*, but as @yeesian pointed out, it costs a lot of time. Note that we also tried to implement (parts) of ISO/IEC 13249-3 SQL/MM Spatial, which adds support for curved geometries. I'm completely open to evolving into a more 3D interface, but let's release this version first. I hope we can evolve to something like PostGIS, they've added support for more 3D things (like volume) and added their own type & functions for a (bounding)box.

* Thought about support for custom dims instead of only hardcoded XYZM, a fancy bounding box type and generalizing/renaming some of the functions in a more Julian way. Only the last one made currently.