Implement a MapCoordsFallible trait #191

urschrei · 2018-03-14T21:56:28Z

This trait is identical to MapCoords, but is intended for use when mapping fallible functions over geometries.

It also adds Failure as a dependency, but I think that's OK.

amandasaurus

What's the point of using a new dependency (failure)? I've seen that crate around in Rust news, but I've never used it nor looked into it. Rust has an Err option, which is what I'd use if I was implementing this.

If there's a 100 point LineString, and you call map_coords_inplace_failible, which is OK for first 10 points, and then fails, won't this make the LineString wrong because only the first 10 will have changed? The only way to work around this is to create a new LineString and std::mem::replace it in, right?

urschrei · 2018-03-15T13:18:20Z

If there's a 100 point LineString, and you call map_coords_inplace_failible, which is OK for first 10 points, and then fails, won't this make the LineString wrong because only the first 10 will have changed? The only way to work around this is to create a new LineString and std::mem::replace it in, right?

I should probably write a test that simulates this, but my intent is that each map call Result is checked. If it's an Error, that bubbles up to the main function via early return.

urschrei · 2018-03-15T13:34:12Z

What's the point of using a new dependency (failure)? I've seen that crate around in Rust news, but I've never used it nor looked into it. Rust has an Err option, which is what I'd use if I was implementing this.

Failure is a lot more flexible than the built-in Err. Using it means we can choose between several different error-handling strategies, and even mix them as necessary. For instance, it's easy to return a generic Error with a String as the cause (which is what I'm doing in #190) if it's coming from an external C lib. But we can also specify Geo specific errors, and because they can be converted into Error, the function signature wouldn't change. Failure's errors can also contain backtrace information, and nest, and it's possible to iterate over nested causes to see exactly what went wrong, and where.
Geo's error-handling story is (imo, and I'm certainly guilty of exacerbating it with unwrap() everywhere) under-specified and lacking at the moment, and this PR (and hopefully #190) both do things that are assumed to break – it seemed like a good time to get the ball rolling on the use of a library which will more than likely become the de-facto error-handling library in Rust.

amandasaurus · 2018-03-15T14:02:42Z

my intent is that each map call Result is checked. If it's an Error, that bubbles up to the main function via early return.

Sure, but since you're modifying the object in place, won't the modifications to the first 10 points still be there? Those can't be undone.

You could change the docs to say "Check the returned error of this, and if it's an error the object is now in an inconsistant state and shouldn't be used"

urschrei · 2018-03-15T14:05:58Z

You could change the docs to say "Check the returned error of this, and if it's an error the object is now in an inconsistant state and shouldn't be used"

Ah, I see what you mean. I had assumed that an Err return value would signal that the operation was unsuccessful, and the result should be discarded, but it's better to be explicit about what the Err means in this context.

urschrei · 2018-03-15T14:22:26Z

Hmm the more I think about the fallible in-place version, the less I like what I've done. @rory is correct here I think – mem::replace is a better idea. If any of the operations do fail, it seems much better to return the unchanged version as an Err. But there's now very little difference to MapCoordsFallible.

This trait is identical to MapCoords, but is intended for use when mapping fallible functions over geometries

Also add note on the consistency of geometries upon which failed operations have been carried out.

frewsxcv · 2018-03-18T13:40:18Z

src/algorithm/map_coords.rs

@@ -59,16 +109,37 @@ impl<T: CoordinateType, NT: CoordinateType> MapCoords<T, NT> for Point<T> {
    }
 }

-impl<T: CoordinateType> MapCoordsInplace<T> for Point<T> {
+impl<T: CoordinateType, NT: CoordinateType> MapCoordsFallible<T, NT> for Point<T> {


instead of having all these explicit MapCoordsFallible impls, what if we had a blanket impl like this (i haven't tried compiling it so it might be wrong):

impl<T: CoordinateType, NT: CoordinateType, G: MapCoords<T, NT>> MapCoordsFallible<T, NT> for G { type Output = G::Output; fn map_coords_fallible( &self, func: &Fn(&(T, T)) -> Result<(NT, NT), Error>, ) -> Result<Self::Output, Error> { Ok(self.map_coords(func)) } }

I don't think we can implement it in terms of map_coords, because of the different input function signatures:
map_coords is func: &Fn(&(T, T)) -> (NT, NT)
map_coords_fallible is func: &Fn(&(T, T)) -> Result<(NT, NT), Error>

And (I think) because we're also missing a try! or ? to check for an early return, somewhere.

yep, you're right 👍

i don't think there's a way to make this work since we can't return from the outer function within the closure if func returs an error

amandasaurus · 2018-03-18T15:30:49Z

I haven't throught about this, but you could probably replace all the MapCoords with calls to MapCoordsInplaceFalliable and appropriate clones and wrapping functions. MapcoordsFaillible is just a clone & MapCoordsInplaceFalliable. Mapcoordsinplace is just a wrapping the function to always return Ok() and then call MapCoordsInplaceFalliable etc etc.

…

On 18 March 2018 15:55:21 CET, "Stephan Hügel" ***@***.***> wrote: urschrei commented on this pull request. > @@ -59,16 +109,37 @@ impl<T: CoordinateType, NT: CoordinateType> MapCoords<T, NT> for Point<T> { } } -impl<T: CoordinateType> MapCoordsInplace<T> for Point<T> { +impl<T: CoordinateType, NT: CoordinateType> MapCoordsFallible<T, NT> for Point<T> { I don't *think* we can implement it in terms of `map_coords`, because of the different input function signatures: `map_coords` is `func: &Fn(&(T, T)) -> (NT, NT)` `map_coords_fallible` is `func: &Fn(&(T, T)) -> Result<(NT, NT), Error>` And (I think) because we're also missing a `try!` or `?` to check for an early return, somewhere.

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

urschrei · 2018-03-18T15:44:47Z

MapcoordsFaillible is just a clone & MapCoordsInplaceFalliable. Mapcoordsinplace is just a wrapping the function to always return Ok() and then call MapCoordsInplaceFalliable etc etc.

Can you sketch this out (or send a PR to my branch)?

As for MapCoordsinplaceFallible, I've been tying myself in knots trying to get it right, and I don't know whether it's worth it – the best case scenario is that it's hiding a clone operation which means that someone (probably one of us) will end up complaining about memory usage and/or slowness at some point.

amandasaurus · 2018-03-18T17:58:36Z

TBH, I'm not convinced that in place failible is a good idea. Leaving an object in a strange/invalid state if one transform fails sounds like a really bad idea, because programmers won't read the docs and it will cause a bug. 🙂 I made the MapCoordsinplace because I didn't want clones, and inplacefailible having "hidden" clones seems bad. So I don't know the solution. We could replace mapcoords with mapcoordsinplace though. I'll do a patch later

…

On 18 March 2018 16:44:48 CET, "Stephan Hügel" ***@***.***> wrote: > MapcoordsFaillible is just a clone & MapCoordsInplaceFalliable. Mapcoordsinplace is just a wrapping the function to always return Ok() and then call MapCoordsInplaceFalliable etc etc. Can you sketch this out (or send a PR to my branch)? As for `MapCoordsinplaceFallible`, I've been tying myself in knots trying to get it right, and I don't know whether it's worth it – the best case scenario is that it's hiding a clone operation which means that someone (probably one of us) will end up complaining about memory usage and/or slowness at some point.

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

urschrei · 2018-03-18T18:00:46Z

I agree, I'll get rid of it.

More trouble than it's worth

frewsxcv · 2018-03-19T01:22:41Z

looks good to me. a little skeptical about incorporating failure, but it seems fine for now and we can always restart that discussion in the future

bors r+

bors · 2018-03-19T01:22:42Z

👎 Rejected by code reviews

frewsxcv · 2018-03-19T01:25:47Z

i guess i should wait for @rory to dismiss their review (or approve the pr). any other concerns?

frewsxcv · 2018-03-19T12:18:03Z

bors r+

191: Implement a MapCoordsFallible trait r=frewsxcv a=urschrei This trait is identical to MapCoords, but is intended for use when mapping fallible functions over geometries. It also adds Failure as a dependency, but I think that's OK.

bors · 2018-03-19T12:25:55Z

Build succeeded

continuous-integration/travis-ci/push

@turboladen

192: Implement rust-proj as an optional feature within geo r=frewsxcv a=urschrei Continuing the discussion from #190 and #32. ## Introduction `rust-proj` is currently (as of `0.3.0`) using PROJ v4.9.x, and depends upon `Geo` for its `Point` type. However, we now have a `proj-sys` crate, with low-level bindings to PROJ v5.0.0. Unfortunately, `rust-proj's` dependency on `Geo` means that we can't directly use its functions within `Geo`, as this introduces a circular dependency (see #190). ### This PR: - Re-implements `rust-proj`'s functionality as an optional feature ("`proj`") within `Geo` using `proj-sys` - it has to be optional, as it requires PROJ v5.0.0 to be present - Makes two new traits –`Project` and `Convert` – available to any geometries implementing the `MapCoordsFallible` trait (see #191), allowing them to be (inverse) projected to and from Geodetic coordinates, and converted between projections. This functionality is a slightly more granular version (in keeping with the new PROJ API, which makes the same distinction) of what's possible using `pyproj` and `Shapely` geometries (see [here](http://shapely.readthedocs.io/en/stable/manual.html#other-transformations) for the official example): ```python from shapely.geometry import Point, LineString import pyproj # LineString in projected coordinates (OSGB36) ls = LineString([(548295.39, 182498.46), (548296.39, 182499.46)]) osgb36=pyproj.Proj("+init=EPSG:27700") eastings, northings = list(zip(*ls.coords)) # inverse-project to geodetic lon, lat coordinates (WGS 84) conv_ls = zip(*osgb36(eastings, northings, inverse=True)) # compare to command-line proj output assert conv_ls == [(0.13774816464635564, 51.52142699952049), (0.1377629902608001, 51.5214357231341)] ``` I propose that we leave `rust-proj` at 0.3.0, clearly marking it as deprecated, directing users to install `Geo` with the `proj` feature if they want v5.0.x features, and linking to `proj-sys` for implementers who don't wish to use `Geo`, for whatever reason. We can also (as per my and @turboladen's discussion in #32) highlight the availability of this feature more clearly, if need be. Co-authored-by: Stephan Hügel <urschrei@gmail.com>

urschrei requested a review from amandasaurus March 14, 2018 21:56

urschrei mentioned this pull request Mar 14, 2018

Implement rust-proj as an optional feature within geo #192

Merged

urschrei force-pushed the fallible_map branch from bc172f0 to 24d9f9d Compare March 15, 2018 13:00

amandasaurus requested changes Mar 15, 2018

View reviewed changes

urschrei added 5 commits March 15, 2018 22:19

Implement a MapCoordsFallible trait

46f70ac

This trait is identical to MapCoords, but is intended for use when mapping fallible functions over geometries

Add fallible in-place map trait

c2056aa

Add test for fallible in-place LineString conversion

157d87e

Also add note on the consistency of geometries upon which failed operations have been carried out.

Fix Failure macro use

cf06880

Add test for fallible mutation success and failure

082261f

urschrei force-pushed the fallible_map branch from 3897cf0 to 082261f Compare March 15, 2018 22:19

Use closure in tests

4710feb

frewsxcv reviewed Mar 18, 2018

View reviewed changes

Remove MapCoordsInplaceFallible trait

11f4c60

More trouble than it's worth

amandasaurus approved these changes Mar 19, 2018

View reviewed changes

bors bot merged commit 11f4c60 into georust:master Mar 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a MapCoordsFallible trait #191

Implement a MapCoordsFallible trait #191

urschrei commented Mar 14, 2018

amandasaurus left a comment

urschrei commented Mar 15, 2018

urschrei commented Mar 15, 2018 •

edited

Loading

amandasaurus commented Mar 15, 2018

urschrei commented Mar 15, 2018

urschrei commented Mar 15, 2018

frewsxcv Mar 18, 2018

urschrei Mar 18, 2018

frewsxcv Mar 19, 2018

amandasaurus commented Mar 18, 2018 via email

urschrei commented Mar 18, 2018

amandasaurus commented Mar 18, 2018 via email

urschrei commented Mar 18, 2018

frewsxcv commented Mar 19, 2018

bors bot commented Mar 19, 2018

frewsxcv commented Mar 19, 2018

frewsxcv commented Mar 19, 2018

bors bot commented Mar 19, 2018

Implement a MapCoordsFallible trait #191

Implement a MapCoordsFallible trait #191

Conversation

urschrei commented Mar 14, 2018

amandasaurus left a comment

Choose a reason for hiding this comment

urschrei commented Mar 15, 2018

urschrei commented Mar 15, 2018 • edited Loading

amandasaurus commented Mar 15, 2018

urschrei commented Mar 15, 2018

urschrei commented Mar 15, 2018

frewsxcv Mar 18, 2018

Choose a reason for hiding this comment

urschrei Mar 18, 2018

Choose a reason for hiding this comment

frewsxcv Mar 19, 2018

Choose a reason for hiding this comment

amandasaurus commented Mar 18, 2018 via email

urschrei commented Mar 18, 2018

amandasaurus commented Mar 18, 2018 via email

urschrei commented Mar 18, 2018

frewsxcv commented Mar 19, 2018

bors bot commented Mar 19, 2018

frewsxcv commented Mar 19, 2018

frewsxcv commented Mar 19, 2018

bors bot commented Mar 19, 2018

Build succeeded

urschrei commented Mar 15, 2018 •

edited

Loading