Skip to content
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

Fix/centroid #155

Merged
merged 7 commits into from Oct 18, 2014
Merged

Fix/centroid #155

merged 7 commits into from Oct 18, 2014

Conversation

awulkiew
Copy link
Member

Fix for Geometries containing big coordinates values.

During the calculation of a centroid the strategy may perform calculus using great values, e.g. add squares of coordinates, which greately increases the numerical error. This is related with #154.

The main idea is to move the Geometry closer to 0 and calculate the centroid for such Geometry. The new origin is the first Point of a Geometry. The translation is done on-the-fly as centroid is calculated. Then the resulting point is translated back.

The picture below shows the Polygon placed closer to 0 and further, plus their centroids before and after the fix.

text3152

… to 0 and then the result back.

The first Point of the Geometry is used as the new origin.
During the centroid calculation each Point is translated by subtracting the origin.
At the end the resulting point is translated back by adding the origin.
…stems than cartesian.

Remove undefined namespace.
Add missing headers.
Add a note about other coordinate systems.
Add copyright info.
@awulkiew
Copy link
Member Author

Even though centroid() doesn't support other CS than cartesian because of lack of strategies I prepared the algorithm by disabling translation in other coordinate systems (see the NOTE in the code).

I enabled the translation for all types of geometries, also non-Areal but I'm not sure is this is needed.
The translation could improve the result in all cases though I'm not sure about it, for non-Areal Geometries only additions and then a division is performed. So the need for transformation probably depends on used Strategy. The transformation could be made a Policy defined by a Strategy or we could just leave it as it is and perform it in all cases. Maybe I should just check if this fix changes/improves the result for e.g. MultiPoint far from 0 containing many Points. If this was the case then I guess we should leave it, if not then think what to do (remove it in algorithm or add a Policy to Strategy). What do you think?

EDIT:

Another solution would be to enclose the translation inside a Strategy, so pass this origin Point into the Strategy::apply() and Strategy::result() and let the Strategy do whatever it likes with it.

But I'm probably as usual going to much ahead with all of this. I guess we could choose a lazy approach here, or rather change only the broken part. So translate only Areal Geometries inside the algorithm and leave everything else as it was before.

@barendgehrels
Copy link
Collaborator

OK to merge this

areal_tag
>::type,
typename CSTag = typename cs_tag<Geometry>::type>
struct translating_transformer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this can go to another file

Add ctor taking Geometry to transformer.
This allows to encapsulate the origin-retrieving-logic inside the transformer.
Remove assign_origin() function.
awulkiew added a commit that referenced this pull request Oct 18, 2014
@awulkiew awulkiew merged commit 1808886 into boostorg:develop Oct 18, 2014
@awulkiew awulkiew deleted the fix/centroid branch October 31, 2014 13:45
@awulkiew awulkiew mentioned this pull request Jan 5, 2015
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.

None yet

2 participants