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

Simplification #18

Merged
merged 150 commits into from
Dec 19, 2021
Merged

Simplification #18

merged 150 commits into from
Dec 19, 2021

Conversation

fillingthemoon
Copy link
Collaborator

Merge simplification branch into master branch

adisidev and others added 30 commits December 23, 2020 20:00
The function returns an array of all graticule intersections.
@mgastner
Copy link
Owner

mgastner commented Jun 4, 2021

When I run

./cartogram -g ../sample_data/bahamas.geojson -v ../sample_data/bahamas_population2010.csv

and then look at cartogram.geojson in mapshaper, there are 65 intersections.

bahamas_intersections

What might be the problem?

@fillingthemoon
Copy link
Collaborator Author

When I run

./cartogram -g ../sample_data/bahamas.geojson -v ../sample_data/bahamas_population2010.csv

and then look at cartogram.geojson in mapshaper, there are 65 intersections.
bahamas_intersections

What might be the problem?

I believe the problem here is that bahamas.geojson and vietnam.geojson have much fewer vertices for their sizes compared to other countries' GeoJSONs. This means that their geometries are already more "simple" than other GeoJSONs. If we try to simplify them to the same degree as other countries' GeoJSONs, we end up with these intersections because there are not enough vertices for crucial areas such as curved areas.

Fortunately, CGAL's simplify function PS::simplify(ct, Cost(), Stop(0.2)); allows us to adjust the degree of simplification, such as 0.2. Though I'm not crystal clear on this, I believe this value should be between 0 and 1 and the lower the number, the fewer resulting vertices there are and so, the bigger the number, the more resulting vertices there are. You can find this line of code on line 953 of simplify.cpp.

I tested this out with a value of 0.5, and so PS::simplify(ct, Cost(), Stop(0.5));. For bahamas.geojson, it resulted in 17 intersections and for veitnam.geojson, it resulted in 9 intersections. Hence, I believe this means that it retains more vertices than if we used 0.2.

If we are able to detect the number of intersections of a GeoJSON, we could potentially do a while loop for simplify.cpp, incrementing that value in each iteration until we reach a desired tolerance of intersections.

@mgastner
Copy link
Owner

mgastner commented Jun 5, 2021

When I run

./cartogram -g ../sample_data/bahamas.geojson -v ../sample_data/bahamas_population2010.csv

and then look at cartogram.geojson in mapshaper, there are 65 intersections.
bahamas_intersections
What might be the problem?

I believe the problem here is that bahamas.geojson and vietnam.geojson have much fewer vertices for their sizes compared to other countries' GeoJSONs. This means that their geometries are already more "simple" than other GeoJSONs. If we try to simplify them to the same degree as other countries' GeoJSONs, we end up with these intersections because there are not enough vertices for crucial areas such as curved areas.

Fortunately, CGAL's simplify function PS::simplify(ct, Cost(), Stop(0.2)); allows us to adjust the degree of simplification, such as 0.2. Though I'm not crystal clear on this, I believe this value should be between 0 and 1 and the lower the number, the fewer resulting vertices there are and so, the bigger the number, the more resulting vertices there are. You can find this line of code on line 953 of simplify.cpp.

I tested this out with a value of 0.5, and so PS::simplify(ct, Cost(), Stop(0.5));. For bahamas.geojson, it resulted in 17 intersections and for veitnam.geojson, it resulted in 9 intersections. Hence, I believe this means that it retains more vertices than if we used 0.2.

If we are able to detect the number of intersections of a GeoJSON, we could potentially do a while loop for simplify.cpp, incrementing that value in each iteration until we reach a desired tolerance of intersections.

This looks like a bug to me. A topological simplification algorithm should not return invalid topologies, no matter what parameters the user sets. The degree of simplification for the Bahamas is already less than what we realistically want. With mapshaper, I can reduce the number of vertices to 20% and still get a topologically valid map. We need to run an example where we isolate a self-intersecting polygon from the Bahamas map and analyse whether the problem is in CGAL or in our code.

@fillingthemoon
Copy link
Collaborator Author

@mgastner

The first screenshot is bahamas.geojson running through the simplification code and the second screenshot is bahamas_grand.geojson (Grand Bamaha only), where I isolated a single polygon and ran it through the simplification code. They both result in the same intersection.

Screenshot 2021-06-05 at 8 16 24 PM
Screenshot 2021-06-05 at 8 18 22 PM

@mgastner
Copy link
Owner

mgastner commented Jun 5, 2021

@mgastner

The first screenshot is bahamas.geojson running through the simplification code and the second screenshot is bahamas_grand.geojson (Grand Bamaha only), where I isolated a single polygon and ran it through the simplification code. They both result in the same intersection.

Screenshot 2021-06-05 at 8 16 24 PM
Screenshot 2021-06-05 at 8 18 22 PM

Let me see whether I understand the situation. Did you create a GeoJSON that contains only one polygon, chosen from one of the polygons that self-intersects in our code? If it still intersects even in isolation, then we must seek help from the CGAL community.

@fillingthemoon
Copy link
Collaborator Author

@mgastner

Sorry for the confusion, I did indeed create a GeoJSON that contains only one polygon, chosen from one of the polygons that self-intersects in our code and it still intersected in isolation.

The following is the GeoJSON file i used!

{
"type": "FeatureCollection",
"name": "bahamas_grand",
"crs": { "type": "name", "properties": { "name": "urn:ogc:def:crs:OGC:1.3:CRS84" } },
"features": [
{ "type": "Feature", "properties": { "fid": 656, "TYPE_1": "District", "ENGTYPE_1": "District", "ABR": "GB", "Island_Gro": "Grand Bahama", "cartogram_id": "10" }, "geometry": { "type": "MultiPolygon", "coordinates": [ [ [ [ 122594.842416608473286, 2947155.341584724839777 ], [ 122291.654872600804083, 2947226.153077695984393 ], [ 122231.389713566517457, 2947042.483519087545574 ], [ 122288.905913269147277, 2947133.505765041336417 ], [ 122399.960992575390264, 2947130.211158112622797 ], [ 122397.212925524567254, 2947037.563923422247171 ], [ 122200.508536974317394, 2946951.094404897652566 ], [ 121955.427253245317843, 2947081.585233204998076 ], [ 122049.323379306704737, 2947449.078677589073777 ], [ 122795.194644965638872, 2947365.134323424659669 ], [ 122791.540339241153561, 2947241.816514134872705 ], [ 122594.842416608473286, 2947155.341584724839777 ] ] ] ] } }
]
}

@mgastner
Copy link
Owner

mgastner commented Jun 5, 2021

@mgastner

Sorry for the confusion, I did indeed create a GeoJSON that contains only one polygon, chosen from one of the polygons that self-intersects in our code and it still intersected in isolation.

The following is the GeoJSON file i used!

{
"type": "FeatureCollection",
"name": "bahamas_grand",
"crs": { "type": "name", "properties": { "name": "urn:ogc:def:crs:OGC:1.3:CRS84" } },
"features": [
{ "type": "Feature", "properties": { "fid": 656, "TYPE_1": "District", "ENGTYPE_1": "District", "ABR": "GB", "Island_Gro": "Grand Bahama", "cartogram_id": "10" }, "geometry": { "type": "MultiPolygon", "coordinates": [ [ [ [ 122594.842416608473286, 2947155.341584724839777 ], [ 122291.654872600804083, 2947226.153077695984393 ], [ 122231.389713566517457, 2947042.483519087545574 ], [ 122288.905913269147277, 2947133.505765041336417 ], [ 122399.960992575390264, 2947130.211158112622797 ], [ 122397.212925524567254, 2947037.563923422247171 ], [ 122200.508536974317394, 2946951.094404897652566 ], [ 121955.427253245317843, 2947081.585233204998076 ], [ 122049.323379306704737, 2947449.078677589073777 ], [ 122795.194644965638872, 2947365.134323424659669 ], [ 122791.540339241153561, 2947241.816514134872705 ], [ 122594.842416608473286, 2947155.341584724839777 ] ] ] ] } }
]
}

Then I think we must unfortunately ask for help (e.g. StackOverflow). Can you prepare a minimum working example (MWE)? We should hardcode the numbers into the MWE so that it is easy to run. We do not need so many decimals for an MWE, and we should also remove as much of our own code as possible. For a single polygon, we do not need any of the polygons-to-linestrings conversion.

@fillingthemoon
Copy link
Collaborator Author

@mgastner

Noted, I can go ahead and prepare a MWE!

@fillingthemoon
Copy link
Collaborator Author

@mgastner

I have pushed an MWE to the branch simplification-testing, in the file simplify_test.cpp here. The whole example is contained in simplify_test.cpp.

Please let me know when I can go ahead and post this on StackOverflow, thanks!

@mgastner
Copy link
Owner

mgastner commented Jun 9, 2021

Thank you for asking for feedback on Stackoverflow. Your proposed workaround works, even with a more aggressive simplification. There was a recent update to Andreas Fabri's answer, but I suppose his bug fix will not be available in the official CGAL release for quite some time.

Before I merge your pull request, can you please

  • fetch the latest changes from the master branch? I would like to be able to see cartograms by the end. You might have to rename mapState to insetState.
  • add a flag (e.g. called --simplify) that is by default off. We may change the default at a later stage of the project, but right now we cannot handle simplified polygons in go-cart.io yet.

@fillingthemoon
Copy link
Collaborator Author

@mgastner

Noted, indeed, I also don't think those fixes will be available any time soon. May I go ahead and mark his answer as the accepted answer then?

And yes, I can go ahead and fetch the latest changes from the master branch. However, may we create a separate branch for the --simply flag task? I'll try to assign this task to another student.

@mgastner
Copy link
Owner

mgastner commented Jun 9, 2021

Noted, indeed, I also don't think those fixes will be available any time soon. May I go ahead and mark his answer as the accepted answer then?

And yes, I can go ahead and fetch the latest changes from the master branch. However, may we create a separate branch for the --simply flag task? I'll try to assign this task to another student.

It is up to you whether you want to accept Andreas Fabri's current answer.

I am hesitant to merge this pull request into the master branch without the --simplify flag. When users fetch changes from this repo, they should not encounter a large difference in the code's behavior if they run it in the same way as before. If you want to create a new branch for the flag, please branch off here and merge the new branch with the simplification branch before requesting a new review.

@fillingthemoon
Copy link
Collaborator Author

@mgastner

Noted, I'll branch off from the simplification branch then.

@mgastner
Copy link
Owner

There is a bug.
./cartogram -g ../sample_data/switzerland.geojson -v ../sample_data/inset_data/inset_switzerland_population2019.csv -s -d
finishes quickly.
However,
./cartogram -g ../sample_data/switzerland.geojson -v ../sample_data/inset_data/inset_switzerland_population2019.csv -s
runs into an endless loop. Both calculations should be the same. We must trace why the two commands behave differently.

@mgastner mgastner merged commit 11a4526 into master Dec 19, 2021
@adisidev adisidev deleted the simplification branch December 3, 2024 12:12
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