Skip to content

Conversation

@barendgehrels
Copy link
Collaborator

This commit adds start turns (if specified in assign policy, and not for rescaling).
They are necessary in cases where the corresponding arrival turn is missed by accuracy.

Testcases (added) are found by recursive_polygons_buffer

Performance: with start turns:

$ recursive_polygons_buffer --count 10000 --level 4 --form triangle --validity 0
Test configuration:
  - Using Start Turns
  - Default test type: d
geometries: 300797 errors: 1501 type: d time: 26.026

Test configuration:
  - Default test type: d
geometries: 291375 errors: 2286 type: d time: 24.516

Note, with rescaling there are 0 errors, so I'm still concentrating on buffer. Next round I will look in some of the other added cases in more detail, but they're not related to missing turns but to clusters.

// p leaves
int const side_pj_q1 = side.pj_wrt_q1();
ui_else_iu(side_pj_q1 == 1, ti);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code is relatively simple now, I first had more conditions but they were not relevant in the end.
But I might find cases where some behaviour should be fine-tuned.


#if defined(BOOST_GEOMETRY_TEST_FAILURES)
BoostGeometryWriteExpectedFailures(1, 1, 2, 3);
BoostGeometryWriteExpectedFailures(1, 14, 2, 11);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because it's related to precision, for float these changes were not necessary.
double/long double add more errors, just because I added the failing cases and part of them are not yet solved.

{
cluster_info& cinfo = nv.second;
auto& indices = cinfo.turn_indices;
std::size_t start_count{0};
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why not std::size_t start_count = 0; ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Habit, it the office we're preferring the new uniform initialization.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I used to be very keen in preferring the uniform initialization heavily and everywhere, until I read the abseil guidelines and tips, in particular https://abseil.io/tips/88

First, “uniform” is a stretch: there are cases where ambiguity still exists (for the casual reader, not the compiler) in what is being called and how.

std::vector<std::string> strings{2}; // A vector of two empty strings.
std::vector<int> ints{2};            // A vector containing only the integer 2.

It's a pity the uniform initialization is not corner-case free improvement to C++.

Copy link
Member

Choose a reason for hiding this comment

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

In this particular case I'd use = 0 (also for numeric classes) because in such case the variable represents 0.

This is not a case when an object of a class is created by passing some abstract which may describe something like number of elements.

From the same reason I'd probably initialize the vector like the one below. In other words to express that the initializer list on the right represents the vector or that it is the vector (just like with 0 being the number):

std::vector<int> ints = {2};

This works right? ;)

And then for all other things I don't have an opinion. I'd probably use the old parentheses. But maybe that's just because it's what I'm familiar with. Maybe with the exception of avoiding double parentheses to deal with the most vexing parse.

So I guess I could also express this as a conservative stance where old initialization is used when possible as it was in the past and new brackets only for new features.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting visions, thanks.
I agree with the statement that it's not that intuitive, int i{0} is quite different from all other languages where it is something like int i=0;
I'll think about it. For now I will merge this, assuming these were not objections.

@barendgehrels barendgehrels merged commit ee3509f into boostorg:develop Nov 4, 2020
@barendgehrels barendgehrels deleted the fix/start-turns branch November 4, 2020 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants