Add coordinate metadata creation and query functions #226
Conversation
urschrei
force-pushed
the
shugel/push-psuusutrpusr
branch
from
9ce8b72 to
9058601
Compare
urschrei
force-pushed
the
shugel/push-psuusutrpusr
branch
from
9058601 to
7776a51
Compare
michaelkirk
left a comment
michaelkirk
left a comment
There was a problem hiding this comment.
This seems important and I'd be glad to see it merged!
It's a little rough around the edges, but if I understand correctly, that seems due to the underlying proj implementation.
| /// # Safety | ||
| /// This method contains unsafe code. | ||
| pub fn coordinate_metadata_get_epoch(&self) -> f64 { | ||
| unsafe { proj_coordinate_metadata_get_epoch(self.ctx, self.c_proj) } |
There was a problem hiding this comment.
Have you had any more luck than me (zero) finding documentation for these methods?
https://github.com/search?q=repo%3AOSGeo%2FPROJ%20proj_coordinate_metadata_get_epoch&type=code
Is epoch in any particular units or are those units specific to each projection?
There was a problem hiding this comment.
"Just" floats of the form year.month,and month can be 0: 2010.3 and 2014.0 are both valid values.
There was a problem hiding this comment.
I couldn't find it documented in proj, but in gdal, which I'm comfortable assuming has the same semantics:
https://gdal.org/en/stable/user/coordinate_epoch.html#coordinate-epoch-support
coordinateEpoch Coordinate epoch as decimal year (e.g. 2021.3), or 0 if not set, or relevant.
So 2021.3 is April 2021.
| /// about the coordinates, such as the epoch they are referenced to. | ||
| /// | ||
| /// # Note | ||
| /// Only **transformation objects** (e.g. those created by calling [`new()`](fn@Proj::new())) can be |
There was a problem hiding this comment.
If you do the wrong thing and try to create coordinate metadata for a pipeline, than you'll hit this, right?:
called
Result::unwrap()on anErrvalue: ProjError("Unknown error (code 4096)")
Should we try to be a "better proj than proj" and encode this in the type system?
I'm ok either way for now.
There was a problem hiding this comment.
I played around with that idea a bit last night. The fundamental issue is that we don't distinguish between a pipeline and a standard transformation object at the type level (neither does libproj), which means we can't get fancy with a bound (I don't think…): everything's a Proj. We have some runtime options:
- Catch the libproj error and return something more informative;
- Check whether the input is a pipeline using proj_info, and return an informative error if kind is Some("Pipeline").
There was a problem hiding this comment.
Right - I was thinking of introducing a bound.
As I understand it, the two "flavors" of the Proj struct have distinct creation methods, right? So we could feasibly return a differently flavored type from each of those methods. Something like Proj<Transform> | Proj<Pipeline>
I'm not sure it's worth the complication though!
There was a problem hiding this comment.
Having a runtime check like you described could be a good compromise. We could at least produce a useful error message.
There was a problem hiding this comment.
I am very much open to something compile-time. But it would be very much a breaking change, right?
There was a problem hiding this comment.
Oh hell yes. We could maybe minimize it by having a default? But definitely breaking.
There was a problem hiding this comment.
I'm also fine to not do it (or do it later).
There was a problem hiding this comment.
OK, we've got a more informative error message. I think rather than breaking everyone for the sake of an operation that isn't required by most users, this will do for now.
urschrei
force-pushed
the
shugel/push-psuusutrpusr
branch
from
7776a51 to
68e582c
Compare
- [x] I agree to follow the project's [code of conduct](https://github.com/georust/.github/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to the project's change log file if knowledge of this change could be valuable to users. - Usually called `CHANGES.md` or `CHANGELOG.md` - Prefix changelog entries for breaking changes with "BREAKING: " --- <s>Draft for now, </s>pls land #226 first.
2 participants
CHANGES.mdorCHANGELOG.md