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.

Sign up for GitHub

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

Add type Bbox and trait BoundingBox #41

Merged
merged 21 commits into from
Jul 21, 2016
Merged

Add type Bbox and trait BoundingBox #41

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7c65977
Implement BoundingBox for LineString
Jul 14, 2016
0337873
Implement BoundingBox for Polygon
Jul 14, 2016
eccc0c9
Substitute &LineString<T> with &Vec<Point<T>> in get_bbox function
Jul 14, 2016
b65d3c0
Implement BoundingBox for MultiPoint
Jul 14, 2016
74a316d
Cleanup test syntax when creating new instances
Jul 14, 2016
49b6172
Implement the Add trait to Bbox
Jul 14, 2016
ce28cca
Implement AddAssign trait for Bbox struct
Jul 15, 2016
2134a69
Implement BoundingBox for MultiLineString and cleanup tests
Jul 15, 2016
de27c9e
Implement BoundingBox for MultiPolygon
Jul 15, 2016
ac69af5
Implement Centroid for Bbox
Jul 15, 2016
12ef2a5
Implement Area for Bbox
Jul 15, 2016
3ecca84
Implement trait Contains for Bbox
Jul 16, 2016
6556567
Implement Intersects trait for Bbox
Jul 16, 2016
8fdb5c9
Merge branch 'generics' into bbox
Jul 16, 2016
8e348a3
Merge branch 'master' into bbox
Jul 17, 2016
ab61011
Cleanup the get_bbox function to remove some duplicate code
Jul 18, 2016
c213d43
Update the get_bbox function removing the else block
Jul 19, 2016
5bae671
Substitute println! with assert_eq! in trait documentation
Jul 19, 2016
9bec1b8
Rewrite get_bbox function to accept as input an IntoIterator instead …
Jul 19, 2016
6f16131
Remove duplicate code in MultiLineString and MultiPolygon, credits to…
Jul 20, 2016
03293f5
Merge branch 'master' into bbox resolving conflicts with PR #43
Jul 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Cleanup the get_bbox function to remove some duplicate code
  • Loading branch information
Zambelli Pietro committed Jul 18, 2016
commit ab6101122a9220dca86d215d337e64abf4eea5f8
38 changes: 17 additions & 21 deletions src/algorithm/boundingbox.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use num::{Float};
use num::Float;

use types::{Bbox, Point, MultiPoint, LineString, MultiLineString, Polygon, MultiPolygon};

Expand All @@ -25,36 +25,32 @@ pub trait BoundingBox<T: Float> {
fn bbox(&self) -> Option<Bbox<T>>;
}


fn get_min_max<T>(p: T, min: T, max: T) -> (T, T)
where T: Float
{
if p > max {(min, p)} else if p < min {(p, max)} else {(min, max)}
}

fn get_bbox<T>(vect: &Vec<Point<T>>) -> Option<Bbox<T>>
where T: Float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can rewrite this function to take an IntoIterator<Item = &Point<T>>. Then, multisomething is just a flat_map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Mon, Jul 18, 2016 at 9:16 AM, Guillaume P. notifications@github.com
wrote:

In src/algorithm/boundingbox.rs
#41 (comment):

+fn get_bbox(vect: &Vec<Point>) -> Option<Bbox>

  • where T: Float

You can rewrite this function to take an IntoIterator<Item = &Point>.
Then, multisomething is just a flat_map.

Sorry I don't understand how I should modify the function, any hints?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not addressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prototype of the function:

fn get_bbox<I, T>(collection: I) -> Option<Bbox<T>>
    where T: Float,
          I: IntoIterator<Item = &Point<T>>

Then, the fold version that I've given should almost work. You can also write an imperative version using for loop.

You can then rewrite MultiSomething as something that look like:

get_bbox(multi.0.iter().flat_map(|poly| poly.0.iter()))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Mon, Jul 18, 2016 at 6:29 PM, Guillaume P. notifications@github.com
wrote:

In src/algorithm/boundingbox.rs
#41 (comment):

+fn get_bbox(vect: &Vec<Point>) -> Option<Bbox>

  • where T: Float

Prototype of the function:

fn get_bbox<I, T>(collection: I) -> Option<Bbox>
where T: Float,
I: IntoIterator<Item = &Point>

Then, the fold version that I've given should almost work. You can also
write an imperative version using for loop.

You can then rewrite MultiSomething as something that look like:

get_bbox(multi.0.iter().flat_map(|poly| poly.0.iter()))

I do like the idea to take as input a Point iterator instead of a vector
that it is much more general. Therefore I think this function should be
refactored following your prototype. Actually I've tried to implement it
myself but I'm new to rust and I'm fighting with lifetime and other compile
errors. If someone with more experience on rust want to work/help on this
I'm open to pull requests or hints :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea: https://is.gd/iYnUSP

If you have any question, don't hesitate to ask.

Copy link
Contributor Author

@zarch zarch Jul 19, 2016 •
edited by TeXitoi
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Jul 19, 2016 at 10:57 AM, Guillaume P. notifications@github.com
wrote:

The idea: https://is.gd/iYnUSP

If you have any question, don't hesitate to ask.

I've worked on the get_bbox function now it is working accepting an
iterator instead of a vector.
But I'm not able to get the MultiLineString working, if I try with:

get_bbox(&self.0.iter().flat_map(|line| line.0.iter()))

this is the error:

src/algorithm/boundingbox.rs:104:9: 104:17 error: the trait bound
`&std::iter::FlatMap<std::slice::Iter<'_, types::LineString<T>>,
std::slice::Iter<'_, types::Point<T>>, [clo
sure@src/algorithm/boundingbox.rs:104:42: 104:62]>: std::iter::Iterator` is
not satisfied [E0277]
src/algorithm/boundingbox.rs:104
        get_bbox(&self.0.iter().flat_map(|line| line.0.iter()))
                                        ^~~~~~~~
src/algorithm/boundingbox.rs:104:9: 104:17 help: run `rustc --explain
E0277` to see a detailed explanation
src/algorithm/boundingbox.rs:104:9: 104:17 note:
`&std::iter::FlatMap<std::slice::Iter<'_, types::LineString<T>>,
std::slice::Iter<'_, types::Point<T>>, [closure@src/algorith
m/boundingbox.rs:104:42: 104:62]>` is not an iterator; maybe try calling
`.iter()` or a similar method
src/algorithm/boundingbox.rs:104:9: 104:17 note: required because of the
requirements on the impl of `std::iter::IntoIterator` for
`&std::iter::FlatMap<std::slice::Iter<'_, t
ypes::LineString<T>>, std::slice::Iter<'_, types::Point<T>>, [closure@src
/algorithm/boundingbox.rs:104:42: 104:62]>`
src/algorithm/boundingbox.rs:104:9: 104:17 note: required by
`algorithm::boundingbox::get_bbox`
error: aborting due to previous error
Build failed, waiting for other jobs to finish...
src/algorithm/boundingbox.rs:104:9: 104:17 error: the trait bound
`&std::iter::FlatMap<std::slice::Iter<'_, types::LineString<T>>,
std::slice::Iter<'_, types::Point<T>>, [clo
sure@src/algorithm/boundingbox.rs:104:42: 104:62]>: std::iter::Iterator` is
not satisfied [E0277]
src/algorithm/boundingbox.rs:104
        get_bbox(&self.0.iter().flat_map(|line| line.0.iter()))
                                        ^~~~~~~~
src/algorithm/boundingbox.rs:104:9: 104:17 help: run `rustc --explain
E0277` to see a detailed explanation
src/algorithm/boundingbox.rs:104:9: 104:17 note:
`&std::iter::FlatMap<std::slice::Iter<'_, types::LineString<T>>,
std::slice::Iter<'_, types::Point<T>>, [closure@src/algorith
m/boundingbox.rs:104:42: 104:62]>` is not an iterator; maybe try calling
`.iter()` or a similar method
src/algorithm/boundingbox.rs:104:9: 104:17 note: required because of the
requirements on the impl of `std::iter::IntoIterator` for
`&std::iter::FlatMap<std::slice::Iter<'_, t
ypes::LineString<T>>, std::slice::Iter<'_, types::Point<T>>, [closure@src
/algorithm/boundingbox.rs:104:42: 104:62]>`
src/algorithm/boundingbox.rs:104:9: 104:17 note: required by
`algorithm::boundingbox::get_bbox`
error: aborting due to previous error
error: Could not compile `geo`.

I've looked around but I was not able to fix it myself.
So I've commit the last working version. Can you explain me what is wrong
with: get_bbox(&self.0.iter().flat_map(|line| line.0.iter()))?

Thank you for your patience and support!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

get_bbox(&self.0.iter().flat_map(|line| line.0.iter()))

you give a reference to an iterator, but you should give directly the iterator that implement IntoIterator.

Thus, just remove the & should fix the problem:

get_bbox(self.0.iter().flat_map(|line| line.0.iter()))

{
if vect.is_empty() {
return None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return is now useless:

None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd better if the return remained, but the else block below was removed. Less indented code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment addressed.

}
if vect.len() == 1 {
return Some(Bbox{xmin: vect[0].x(), ymax: vect[0].y(),
xmax: vect[0].x(), ymin: vect[0].y()})
} else {
let (mut xmax, mut xmin) = (T::neg_infinity(), T::infinity());
let (mut ymax, mut ymin) = (T::neg_infinity(), T::infinity());
for pnt in vect.iter() {
let mut xrange = (vect[0].x(), vect[0].x());
let mut yrange = (vect[0].y(), vect[0].y());
for pnt in vect[1..].iter() {
let (px, py) = (pnt.x(), pnt.y());
if px > xmax {
xmax = px;
} else if px < xmin {
xmin = px;
}
if py > ymax {
ymax = py;
} else if py < ymin {
ymin = py;
}
xrange = get_min_max(px, xrange.0, xrange.1);
yrange = get_min_max(py, yrange.0, yrange.1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be written using a fold. That's just personal preference, so change it if you prefere but keep it like that if you prefere this imperative form.

Some(Bbox{xmin: xmin, ymax: ymax,
xmax: xmax, ymin: ymin})
Some(Bbox{xmin: xrange.0, xmax: xrange.1,
ymin: yrange.0, ymax: yrange.1})
}
}


impl<T> BoundingBox<T> for MultiPoint<T>
where T: Float
{
Expand Down Expand Up @@ -119,7 +115,7 @@ impl<T> BoundingBox<T> for MultiPolygon<T>
where T: Float
{
///
/// Return the BoundingBox for a MultiLineString
/// Return the BoundingBox for a MultiPolygon
///
fn bbox(&self) -> Option<Bbox<T>> {
let vect = &self.0;
Expand Down