-
Notifications
You must be signed in to change notification settings - Fork 12
Implement the MovingFeatures Spec #24
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
Conversation
|
I'm not yet sure if it's worth it to convert from columnar representations in TemporalPrimitiveGeometry for datetimes, coordinates and orientations to row-based representation, mainly to enforce same length arrays. |
|
I'm also still unsure on how to correctly represent Crs and Trs which appear to differ from other OGC API Specs |
|
Hi @KrausMatthias, thanks for the contribution!
I would prefer some other way to avoid writing too much custom serialization code.
We probably have to define an explicit Crs and Trs struct for this. |
|
Thanks for the feedback :) I'm now using serde(try_from ) to check for same length Vecs, which is much less complex, but slightly ugly, maybe I can come up with a nicer name than "SameLengthDateTimeCoordinatesVecs" 😄
|
I prefer this mouthful given how much code is saved. It is debatable whether we even want to enforce it. Obviously, it is an improvement, however, there is generally no validation done regarding the values of types. For example, we do not check whether a certain CRS code actually exists. Also, not sure if this should be part of the serialization or an additional layer/function. Though this is something for another time, let's keep it as is for now. |
a8bdbf6 to
8a02b9f
Compare
I think there is a difference between CRS codes and same length vecs, as we don't know which projections the application will support, but for mismatched datetimes/coordinates vecs we do know it's not possible to reliably construct a trajectory from that (besides this constraint is explicitly stated in the spec). So I would prefer to keep validation. I've now also started adding temporal properties, but I'm not very happy about that so far as it's a painful mix of almost equivalent types. I will try to clear this up a bit in another commit. |
e6d0f95 to
9364110
Compare
|
I've now added the transactions traits to ogcapi-drivers and created a minimal blanket implementation based on the FeatureTransactions for testing. The idea is to first get a spike done in ogcapi-services for some client side testing before adding a proper implementation in postgres. I guess for S3 it might already be good enough. |
59e6440 to
9882cfd
Compare
@KrausMatthias that's great! Indeed, the services are intended mainly for testing. The current S3 backend is due for some revamping. I intend to rework it with the object store crate at some point if time allows. Anyways, how about landing the types and dealing with outstanding issues in a follow pr? |
|
I still want to cleanup some details about the types, but I guess then they are ready.
Unrelated question, why do we enforce Utc and not just any Timezone (As long as we have a timezone)? Is this required in the spec somewhere? |
This could maybe be simplified by custom serialization on the struct/enum, mimicking what serde is doing, and then just add a test on the respective attributes to check for the same length.
Up to you, I am not familiar enough to assess the implications.
From a quick look, I would assume
Honestly, I do not recall anymore. Probably some (draft) specifications emphasized it. Just take what you think is most appropriate for Moving Features. We can revise this at some point, hopefully based on OGC consolidation work and not ours. |
…l primitive geometry
Not sure if all of this is correct as it's a wild mix of almost identical types.
…lattend due to flattening of 'Value'
ae3843c to
14b4f0c
Compare
|
Thanks for your patience, I finally found the time to pick this up again |
No problem, happy to review it tomorrow. It's already past midnight here. |
b4l
left a comment
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.
Some minor remarks, otherwise looks good!
|
Hi @KrausMatthias, I would like to merge this. From my side, there is only one question open: whether we should always error on mismatching vec length. Do you have any preference for this? I am leaning towards failing fast and always enforcing it. |
|
I agree, I would also go for failing fast |
|
@KrausMatthias Thanks for keeping at it, I think it's a great addition! |
CHANGES.mdorCHANGELOG.mdFirst stab at implementing MovingFeatures, starting with Primitive Temporal Geometry.
Would be happy about early feedback :)