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 bounding box #1546
Spatial join bounding box #1546
Changes from 5 commits
e9ed3e4
ada2978
eeadcd7
127ba53
dcb60dd
be09a86
c4ddec6
122372b
12f3c13
213b02e
ecda249
97d039b
603ec46
3831da8
9ffaa0a
032eade
d50e2cc
24e31c6
8285355
56e1618
7d8b98b
89491a1
8743986
23f2ed2
ce5db45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 354 in src/engine/SpatialJoin.cpp
Codecov / codecov/patch
src/engine/SpatialJoin.cpp#L354
Check warning on line 235 in src/engine/SpatialJoinAlgorithms.cpp
Codecov / codecov/patch
src/engine/SpatialJoinAlgorithms.cpp#L234-L235
Check warning on line 253 in src/engine/SpatialJoinAlgorithms.cpp
Codecov / codecov/patch
src/engine/SpatialJoinAlgorithms.cpp#L251-L253
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.
This I don't understand,
you return the antiBoundingBox from the
boundingBox
function, how do you then know outside that it is an antiBoundingBox?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.
Actually the computeAntiBoundingBox function returns a set of boxes, which cover the entire earth, except for the anti bounding box. It way just the naming, which was very confusing. I renamed it to computeUsingAntiBoundingBox and added what i just said in the comment of the 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.
The
maxDistbuffer * 360 / circumferenceMax
can be an explicit variable with an explicit name.(what is it, the maximal number of degrees that the maxDist can lead to?
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.
yes, it's the distance but instead of beeing represented in meters it is represented in degrees. I made a variable for it to avoid calculating it twice
Check warning on line 325 in src/engine/SpatialJoinAlgorithms.cpp
Codecov / codecov/patch
src/engine/SpatialJoinAlgorithms.cpp#L324-L325
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 have these corrections also elsewhere, maybe you can extract a function for this.
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 think that's possible, because this is the only general implementation. The other cases require extra flags, which get set, when a certain condition happens (i.e. the flag 'northPoleTouched'). I could implement the function in a way, that it returns the flags as well, but then all places would need dummyVariables, which accept the other flags. I think in total it would make the code less readable
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 have to measure, It might be faster to create the index for the bigger IdTable.
But as everything else is symmetric, you should be able to easily switch this for experiments.
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.
that's a point, which is already on my list for evaluations. I would postpone it until the evaluation phase if that's fine with you
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.
Absolutely.
Check warning on line 439 in src/engine/SpatialJoinAlgorithms.cpp
Codecov / codecov/patch
src/engine/SpatialJoinAlgorithms.cpp#L434-L439
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 think if you have all the points in advance (which you do here), there is a better algorithm in boost (the "packing algorithm"), but this is also maybe a future improvement for your evaluation phase.
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.
yes, that on my list for the evaluation phase. If it's fine with you i will postpone it until then
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.
Maybe this can be a small helper function (geoPointToBoostPoint).
Otherwise, same comment as above: what about inputs that are not points.
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 think the error handling could be, that the point is skipped and a warning is printed. But in that case this helper function would need to return a std::optional (in case the conversion didn't work), which would make the rest of the code less readable, because of all the .value() calls.
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.
Can you use a vector of pointers or something of the like as the type of the
results
vector to save memory?We should make the large data structures respect the memory limit (all classes that take an allocator can take
ad_utility::AllocatorWithMemoryLimit
, see how other operations do this.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.
Check warning on line 469 in src/engine/SpatialJoinAlgorithms.cpp
Codecov / codecov/patch
src/engine/SpatialJoinAlgorithms.cpp#L467-L469