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

Make Test Geometry re-usable in other packages. #75

Closed
wants to merge 2 commits into from

Conversation

evetion
Copy link
Member

@evetion evetion commented Nov 4, 2022

Moved all the test geometry from the tests to the package itself, so other package developers can reuse it. Renamed all MyXGeom to XGeom.

@evetion evetion requested a review from rafaqz November 4, 2022 10:20
@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

This should prevent a lot of boilerplate in other packages.

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

Beat me to it.

But if we are including these, can we just make them real objects so users can also use them? Currently there are a lot of objects that are awkward to define anywhere else.

@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

Can you give an example? I don't follow.

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

I mean for creating geometry objects in scripts the simplest way currently seems to be using GeometryBasics.jl Point Polygon etc.

But it's not the whole set of traits. For that you have to use ArchGDAL or something, but creating C pointer objects is pretty heavyweight approach as a default. I would like to just have basic wrappers for all the GeoInterface traits defined here for use in both testing and scripts.

@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

Ah, the blessed, actually useable geometries. Wellknowngeometry is normally used for that, but I made it into an interface exchange only.

Currently working on flatgeometry based on geoarrow, that might be the best future option.

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

Hmm, still has dependencies though?

I mean dirt simple, 100 lines of julia, no overhead wrapper types you can use and have converted to other external types.

@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

You probably looked at flatgeobuf, that's a different beast. FlatGeometry is not public yet, but is based on the geoarrow spec (in development), which is similar to shapefile in how it stores things as flattened arrays of coordinates.

I'm all for dirt simple, although as soon as you add helpful constructors, conversion, I will exceed that 100 lines 😉

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

I'm ok with 150 ;)

(it also sounds like FlatGeometry is a format that is doing transformations to the data. It would be great to have something that can use any geointerface compatible points, linestrings or any isgeometry objects to build up more complicated objects just using wrappers and vectors - I only need lazy wrappers instead of actual data formats)

@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

Such wrappers could live in GeoInterface actually, but it's probably harder than you describe it. FlatGeometry is indeed an actual data format, but still, could be the default eventually for most GIS stuff.

Not sure if creating a MultiPoint with a WKB Point, WKT Point and JSON3 point (Integer), and maybe with different coordinate dimensions/names) is easy or stable.

In the meantime, we might merge this PR as is?

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

Im not sure what will go wrong with that honestly. But the goal would be to support using objects from any package, not from all packages at once.

But can I make a PR to make these real geometries over the weekend? If we start using them in other packages we cant really remove them afterwards.

@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

I'm happy with such a PR. Although I think that these have merit on their own, as dummies for testing (and are documented as such). Of course they could be backed by your PR geometries, but that wouldn't even need to be breaking.

I think the distinction here is that these dummies can be instantiated without any need to provide actual points or coordinates, which can be a pain for (multi)geometries.

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

Yeah that is also useful

@visr
Copy link
Member

visr commented Nov 4, 2022

But if we are including these, can we just make them real objects so users can also use them?

Right now they are in a submodule TestGeometry, making it quite clear they are not meant for end users, only as a testing utility. If we want this we probably should go further and document that they are not part of GeoInterface's semver. Perhaps it shouldn't even be in GeoInterface.jl, although it's only 100 lines. A separate package or even just putting them in GeoInterfaceRecipes could also work.

If we are talking about creating real usable objects for end users it's quite a different discussion, and we shouldn't quickly throw a set of structs in the interface package.

@evetion
Copy link
Member Author

evetion commented Nov 4, 2022

Im not sure what will go wrong with that honestly. But the goal would be to support using objects from any package, not from all packages at once.

Sounds great, and would fill in the gaps for when you don't know how the package builds up its geometries, but you'd end up with a competing geometry, that hopefully could be converted to its package equivalent. (as in, is a GeoInterface.MultiPoint of two PackageA.Points equivalent (and convertable) to a PackageA.MultiPoint?

In a sense we're missing an construction interface. Something like construct(trait, customgeoms...). The current convert has the drawback of not knowing the actual types to convert to, i.e. I want the type that implements PolygonTrait from package A.

@rafaqz
Copy link
Member

rafaqz commented Nov 4, 2022

(as in, is a GeoInterface.MultiPoint of two PackageA.Points equivalent (and convertable) to a PackageA.MultiPoint?)

Yeah that's the idea, convert should just work where PackageA has a user constructable MultiPoint object, or like GeoJSON just not care what kind of object you pass to write as long as it follows the interface. It may not be as efficient as using the package directly where that is possible. Sometimes you cant actually construct PackageA.MultiPoint in a reasonable way anyway, so its probably most often useful in preparing an object for use in PackageC.somefunction that wants a MultiPointTrait geom.

@rafaqz
Copy link
Member

rafaqz commented Nov 6, 2022

Also want to point out that these geometries are pretty limited for testing. @yeesian has asked for tests in #74 but we can't actually test geometries with length 0 or 1 as the lengths are fixed.

@visr
Copy link
Member

visr commented Nov 6, 2022

With a separate GeoInterfaceTests we could really offer rigorous implementation tests that cover different geometries, and not have to worry about adding code to a lightweight interface package. In the tests of that package we could even add ArchGDAL, GeometryBasics and other important implementations to make sure that things like conversion work well.

@rafaqz
Copy link
Member

rafaqz commented Nov 6, 2022

Yeah I think that's better. Adding test only objects to the runtime code here doesn't feel great either.

@evetion
Copy link
Member Author

evetion commented Mar 27, 2023

Superseded by #78

@evetion evetion closed this Mar 27, 2023
@rafaqz rafaqz deleted the feat/testgeometry branch August 4, 2023 13:01
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.

3 participants