Test set operations on random grids with integer coordinates #1361
Conversation
|
Cool! I'll review it Wednesday |
tinko92
force-pushed
the
test/random-integer-grid
branch
from
b838f8e to
afd9460
Compare
| @@ -25,4 +25,4 @@ exe recursive_polygons : recursive_polygons.cpp ; | |||
| exe star_comb : star_comb.cpp ; | |||
|
|
|||
| exe ticket_9081 : ticket_9081.cpp ; | |||
|
|
|||
| exe random_integer_grids : random_integer_grids.cpp ; | |||
There was a problem hiding this comment.
Can you added it to the CMakeLists.txt too?
| out = temp; | ||
| } | ||
| } | ||
| const auto b_gs = [&settings](bits const& b) { return std::make_pair(b, settings); }; |
There was a problem hiding this comment.
Finally got it, bit_gridsettings probably.
tinko92
force-pushed
the
test/random-integer-grid
branch
from
afd9460 to
ce38ac2
Compare
|
|
||
| namespace bg = boost::geometry; | ||
|
|
||
| using point = bg::model::d2::point_xy<int>; |
There was a problem hiding this comment.
You probably mean here: <BOOST_GEOMETRY_DEFAULT_TEST_TYPE>, though it's a bit long.
It's actually a pity that it can only test integers (though you gave the rationale). I would like to have a version for double as well. But we can also change it later.
| return os; | ||
| } | ||
|
|
||
| bits geometry_to_bits(mp const& g, std::vector<point> const& tp) |
There was a problem hiding this comment.
The names are short and sometimes cryptic.
I don't say I never use abbreviations myself (I do). But these ones I don't fully get all.
tp, what is it?
b is probably fine in this context.
g is short (but at least I understand it), I would use geometry here or make the mp a multi_polygon_t or mp_t and call it mp then here
bm (below)
prng and gen, I would use longer names
| if ( ! bg::is_valid(test_geo) ) | ||
| { | ||
| std::cout << op_label << "(\n\t " << bg::wkt(geo1) << ",\n\t " << bg::wkt(geo2) << "\n), " | ||
| << "generated from" << b_gs(bits1) << "and" << b_gs(bits2) << "is invalid.\n\n"; |
There was a problem hiding this comment.
Looks like some spaces are missing, do you have a sample output?
There was a problem hiding this comment.
I get them now, it's fine because the b_gs lambda generates new lines
| int width = 5; | ||
| int height = 5; | ||
| int count = 1; | ||
| prng::result_type seed = prng::default_seed; |
There was a problem hiding this comment.
Can we generate it somehow from current time?
| po::value<decltype(settings.width)>(&settings.width)->default_value(settings.width), | ||
| "Width of grid") | ||
| ("height", | ||
| po::value<decltype(settings.height)>(&settings.height)->default_value(settings.height), |
There was a problem hiding this comment.
The decltype is elegant. But because it's an int, if I specify -10 for height, I get a segmentation fault...
| catch(...) | ||
| { | ||
| std::cout << "Other exception" << '\n'; | ||
| } |
There was a problem hiding this comment.
I would like to see a summary of the number of tests, the number of failures, and the time spent.
There was a problem hiding this comment.
+1 here. EDIT: I mean it would be interesting to see some stats for the generated polygons (validity, failures etc).
This is completely fine, indeed the robustness tests are to be executed manually. I do it regularly (not all of them, but some). I'll continue tonight or tomorrow - I had some remarks but basically it looks good, I'm glad with the addition. |
tinko92
force-pushed
the
test/random-integer-grid
branch
2 times, most recently
from
4131e98 to
27dfc3a
Compare
|
The second commit was intentionally not squished to delineate the new changes from the already reviewed ones but I can rebase and force push if a single commit PR is prefered. |
I usually look at |
| [](int acc, auto const& kv) { return acc + kv.second; }); | ||
| std::cout << "\niterations: " << settings.count | ||
| << " errors: " << failure_count | ||
| << " time: " << elapsed_ms/1000 << '\n'; |
There was a problem hiding this comment.
suggestion: elapsed_ms / 1000.0 to avoid 0
| } | ||
| if(settings.height < 1 || settings.width < 1) | ||
| { | ||
| std::cout << "Invalid dimensions, height and width need to be positive.\n"; |
There was a problem hiding this comment.
Thanks for handling my comments! I'm OK with this PR and will approve,
but I prefer is @vissarion has a look too.
tinko92
force-pushed
the
test/random-integer-grid
branch
from
27dfc3a to
c4ac51c
Compare
|
|
||
| std::ostream& operator<<(std::ostream& os, std::pair<bits, grid_settings> const& b_gs) | ||
| { | ||
| if(b_gs.second.verbose) |
There was a problem hiding this comment.
coding style: please leave a space after if
There was a problem hiding this comment.
there are a few others like this in this file, please check
| else | ||
| { | ||
| os << '{' << b_gs.first[0].to_ullong(); | ||
| for(size_t i = 1; i < b_gs.first.size(); ++i) os << ' ' << b_gs.first[i].to_ullong(); |
There was a problem hiding this comment.
coding style: please leave a space after for
| catch(...) | ||
| { | ||
| std::cout << "Other exception" << '\n'; | ||
| } |
There was a problem hiding this comment.
+1 here. EDIT: I mean it would be interesting to see some stats for the generated polygons (validity, failures etc).
| } | ||
| if (! test_all(settings)) return 1; | ||
| } | ||
| catch(std::exception const& e) |
There was a problem hiding this comment.
please also add a space after catch here
| { | ||
| std::cout << "Exception " << e.what() << '\n'; | ||
| } | ||
| catch(...) |
|
|
||
| namespace bg = boost::geometry; | ||
|
|
||
| using point = bg::model::d2::point_xy<BOOST_GEOMETRY_DEFAULT_TEST_TYPE>; |
There was a problem hiding this comment.
please remove column align from here too
tinko92
force-pushed
the
test/random-integer-grid
branch
from
c4ac51c to
2537c6b
Compare
|
Thanks for the comments, @vissarion (I think some of the early ones maybe overlapped with my previous push) and @barendgehrels ! (Sorry about that 50c20ad push. I wasn't aware of that cmake test running on my private fork, or I would have run it locally to catch that before pushing). |
tinko92
force-pushed
the
test/random-integer-grid
branch
from
2537c6b to
50c20ad
Compare
As discussed in #1356 , this adds an overlay test from https://gist.github.com/tinko92/fad450e1058ce8ceebde66a6b86d45c1 that generates grids based on bit patterns and verifies the correctness of set operation by testing against the corresponding logical operation on the bit sets.
The test is cleaned up and generalized to take width and height options from the command line. It is not intended to be run by default as part of the test suite (I understand, this is not the case for the robustness tests in general?) because it is only useful with a large iteration count and thereby long-running. It is also meant to be built with optimizations to mitigate the high time cost and prints failures to stdout in WKT with the corresponding bitset rather than triggering assertions.
The design idea was to build a test that, in principle, could test every possible code path in overlay (for sufficiently large width and height). I lack the insight in the overlay details to judge whether it meets that goal. It is also meant to test only for non-numerical issues, so it intentionally only supports integer coordinates. The limitation to a rectangular grid is just for simplicity of implementation, but in principle every indexable, disjoint set of rings with corresponding test points should work.