add union operation for geometry #379

zredb · 2023-03-10T04:29:24Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This is my first time to commit code to this repository. I see that vecotr only has an intersection operation at present, so I try to add a union operation to it. If it can be accepted, I will add other operations in the same way later.

metasim

@zredb Thank you for the contribution!
Could you please add documentation to union, similar to intersection?

metasim · 2023-03-13T18:20:58Z

Also, I'm not seeing an update to CHANGES.md.

lnicola · 2023-03-13T18:27:40Z

src/vector/ops/set.rs

+        unsafe {
+            let ogr_geom = gdal_sys::OGR_G_Union(self.c_geometry(), other.c_geometry());
+            if ogr_geom.is_null() {
+                return None;


I haven't tested, but it might be possible to return a proper error here?

But then I'm not sure what we should do in the !has_gdal_ptr() case...

According to gdal's document, when an error occurs, the return value is null, so here we return None, which seems to be consistent with gdal.

Yeah, but in most cases GDAL will set an error code and message like here, even without documenting it explicitly.

But indeed, this doesn't seem to be done in a consistent fashion.

lnicola · 2023-03-13T18:28:16Z

src/vector/ops/set.rs

+
+        let inter = inter.unwrap();
+
+        assert_eq!(inter.area(), 135.0);


I think we have some assert_almost_eq helper somewhere.

lnicola · 2023-03-13T18:30:36Z

.gitignore

@@ -8,3 +8,4 @@
 GPATH
 GTAGS
 GRTAGS
+/setenv.bat


Do we want this? In the long run every contributor might end up with a script like that, but with a different name.

Setenv is just my own configuration. I have removed it from gitignore.

add change log to CHANGES.md

lnicola · 2023-03-14T14:43:27Z

.gitignore

@@ -7,4 +7,4 @@
 # gtags
 GPATH
 GTAGS
-GRTAGS
+GRTAGS


Nit: your IDE is eating final newlines.

lnicola · 2023-03-14T14:43:35Z

src/vector/ops/set.rs

+    ///
+    /// # Returns
+    /// Some(Geometry) if both Geometries contain pointers
+    /// None if either geometry is missing the gdal pointer, or there is an error.


Suggested change

/// None if either geometry is missing the gdal pointer, or there is an error.

/// None if either geometry is missing the GDAL pointer, or there is an error.

metasim · 2023-03-14T15:22:50Z

src/vector/ops/set.rs

+    ///
+    /// # Returns
+    /// Some(Geometry) if both Geometries contain pointers
+    /// None if either geometry is missing the gdal pointer, or there is an error.


Suggested change

/// None if either geometry is missing the gdal pointer, or there is an error.

/// `None` if either geometry is missing the gdal pointer, or there is an error.

I note that the documentation for intersection isn't that great. The docs in predicates.rs is closer to the mark. Would be great if you can bring it to that level, but if you can't I'll do a separate PR to normalize it.

lnicola · 2023-03-14T15:24:05Z

src/vector/ops/set.rs

+    /// otherwise the result might be wrong.
+    ///
+    /// # Returns
+    /// Some(Geometry) if both Geometries contain pointers


Suggested change

/// Some(Geometry) if both Geometries contain pointers

/// `Some(geometry)` if both Geometries contain pointers

? 😄

lnicola

If it's not too much of a hassle, would you mind squashing?

zredb · 2023-03-16T00:46:53Z

I think I would, but what does "squaring" mean? English is not my first language, so I don't quite understand what "squaring" really means.

lnicola · 2023-03-16T13:55:22Z

Squashing, as in merging the 5 commits into a single one. Something like:

$ git reset @~5 # 5 commits before, or git reset 1dbc60215033caee1f99d933ae11354f70bc5f0e, the last commit before your changes
$ git commit -a
$ git status # make sure everything looks fine
$ git log # make sure everything looks fine
$ git push -f

(these commands reset your branch to the commit before your changes, leave your changes uncommitted in the working directory, then make a new commit with them)

But you don't need to worry about it.

I'll let metasim take another look before merging.

metasim · 2023-03-16T15:23:36Z

bors r+

bors · 2023-03-16T15:32:58Z

Build succeeded:

394: Updated `set.rs` docs to be more consistent with those in `predicates.rs`. r=metasim a=metasim - [X] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md). - [X] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- <img width="974" alt="image" src="https://user-images.githubusercontent.com/131013/236706192-fad8a481-5cc4-476d-9429-eedf66d419b8.png"> Context: #379 (comment) Co-authored-by: Simeon H.K. Fitch <[email protected]>

zhb added 3 commits March 10, 2023 12:09

add union function for geometry

d654387

add union function for geometry

f2c2565

reformat code.

9205122

metasim requested changes Mar 13, 2023

View reviewed changes

lnicola reviewed Mar 13, 2023

View reviewed changes

add documentation for Geometry::union method;

26d3743

add change log to CHANGES.md

lnicola reviewed Mar 14, 2023

View reviewed changes

.gitignore Outdated

@@ -7,4 +7,4 @@

# gtags

GPATH

GTAGS

GRTAGS

GRTAGS

Copy link

Member

lnicola Mar 14, 2023

Choose a reason for hiding this comment

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

Nit: your IDE is eating final newlines.

lnicola reviewed Mar 14, 2023

View reviewed changes

metasim reviewed Mar 14, 2023

View reviewed changes

lnicola reviewed Mar 14, 2023

View reviewed changes

fix documentation error.

cb62db5

lnicola approved these changes Mar 15, 2023

View reviewed changes

zredb requested a review from metasim March 16, 2023 00:40

metasim approved these changes Mar 16, 2023

View reviewed changes

bors bot merged commit 7b874de into georust:master Mar 16, 2023

metasim mentioned this pull request May 7, 2023

Updated set.rs docs to be more consistent with those in predicates.rs. #394

Merged

2 tasks

metasim mentioned this pull request May 18, 2023

Release 0.15 #399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add union operation for geometry #379

add union operation for geometry #379

zredb commented Mar 10, 2023

metasim left a comment

metasim commented Mar 13, 2023

lnicola Mar 13, 2023

zredb Mar 14, 2023

lnicola Mar 14, 2023

lnicola Mar 13, 2023

lnicola Mar 13, 2023

zredb Mar 14, 2023

lnicola Mar 14, 2023

lnicola Mar 14, 2023

metasim Mar 14, 2023

metasim Mar 14, 2023

lnicola Mar 14, 2023

lnicola left a comment

zredb commented Mar 16, 2023

lnicola commented Mar 16, 2023 •

edited

Loading

metasim commented Mar 16, 2023

bors bot commented Mar 16, 2023

@@ @@ -7,4 +7,4 @@ @@
               # gtags
               GPATH
               GTAGS
-              GRTAGS
+              GRTAGS

	/// None if either geometry is missing the gdal pointer, or there is an error.
	/// None if either geometry is missing the GDAL pointer, or there is an error.

	/// None if either geometry is missing the gdal pointer, or there is an error.
	/// `None` if either geometry is missing the gdal pointer, or there is an error.

	/// Some(Geometry) if both Geometries contain pointers
	/// `Some(geometry)` if both Geometries contain pointers

add union operation for geometry #379

add union operation for geometry #379

Conversation

zredb commented Mar 10, 2023

metasim left a comment

Choose a reason for hiding this comment

metasim commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnicola left a comment

Choose a reason for hiding this comment

zredb commented Mar 16, 2023

lnicola commented Mar 16, 2023 • edited Loading

metasim commented Mar 16, 2023

bors bot commented Mar 16, 2023

lnicola commented Mar 16, 2023 •

edited

Loading