Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Spatial join area #1713
base: master
Are you sure you want to change the base?
Spatial join area #1713
Changes from 5 commits
c7a1ea8
a266201
2cc8504
d784513
f6e1647
1a4ff8f
e3a2644
f38cb1d
4d71c4e
38c9585
ddea6d3
1902d44
31e7054
cba4324
d5c934c
c1dc963
c5284ef
fc1a1fe
ec75e89
4fd698c
173e646
7dd8156
1c60bf9
3b8ea1a
a121ef2
1168f32
ca7f08e
9d2956e
14032bd
29171b5
457f8d4
bf886c4
10bdf7b
5f12f22
978b512
4132701
d711586
f3c5b01
a46fccd
2cc0fef
96a903a
47ba454
61c6b98
c0bf282
988ba8d
e42d9d0
5b88f47
5a99080
a7f4695
e5121f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 50 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L49-L50
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.
What exactly is the multiplication doing here, the magic constant should be explained.
Is the function name actually
computeDistanceInMeters
(you first compute it, and then do some magic multiplication to convert)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.
a very verbose explanation of this is in the header file. But you're right, the function should be called different, as it first gets the distance in degrees and then converts it. I called it computeDist, since that's what the other compute distance functions are called. Then it's consistent and depending on the parameters available, one can call one or the other computeDist function
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.
Thank you for the clarification, I am much less confused now:)
Check warning on line 95 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L93-L95
Check warning on line 122 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L121-L122
Check warning on line 128 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L127-L128
Check warning on line 132 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L130-L132
Check warning on line 135 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L134-L135
Check warning on line 137 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L137
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.
What is stored e.g. in
geo1.geometry.value()
ifgeo1
points to aPOINT
already? This last part looks a bit confusing.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.
one line above, there is a comment, which states "one point and one area". So this is the case, where we have one GeoPoint and one area. Then geom1 and geom2 get initialized to be the geometries (which is wrong in one case. When the geometry is a GeoPoint, then geomX.geometry is std::nullopt). The if below checks, which of those two is not a geometry but a geopoint. Then it converts geopoint to boost point (as boost requires both types to be boost geometries). You're right, that they should not be initialized with .value, since that is std::nullopt for at least one of them. I still have to write a test, which checks that, but it should work now
Check warning on line 143 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L139-L143
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.
Easier control flow:
if (point.has_value) { return point; } else { return getAnyGeometry(...)} Everything else then is not nested in the
if`.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.
'(with my suggestion for the
AnyGeometry
)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.
i don't understand what you mean
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.
You can drop the
std::optional
if you explicitly set the return type of the lambda tostd::optional<AnyGeometry>
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.
Of course only relevant, if we still have this code at all.
Check warning on line 163 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L162-L163
Check warning on line 166 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L165-L166
Check warning on line 212 in src/engine/SpatialJoinAlgorithms.cpp
src/engine/SpatialJoinAlgorithms.cpp#L211-L212