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

Simplify exceptions for aruba #1179

Merged

Conversation

pascalheraud
Copy link
Contributor

Fixes #1153

I added a parameter "lessSimplify" to zones to avoid too much simplification on small zones like Aruba.

Diffing world.json is very hard due to huge single line. As far as I can see, it only adds Aruba. I did not notice any other changes.

I also added unzip to docker container that is required by topogen.sh. Some other various files needed for development using container were added too.

Another point is the third_party_maps folder that is missing inside container and causes download map not working.

This PR works well. Naming and comments can be changed according to your coding standards.

@corradio corradio self-requested a review March 6, 2018 06:33
Copy link
Member

@corradio corradio left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
Can you post a screenshot just to be sure it works?

zoneDefinitions.forEach(zone => {
// Take only zones being small simplified
if (('lessSimplify' in zone) && zone.lessSimplify) {
if (zone.zoneName in zonesLessSimplified || zone.zoneName in zones)
Copy link
Member

Choose a reason for hiding this comment

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

Identation problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -27,3 +27,8 @@ services:
- './web/webpack.config.js:/home/web/webpack.config.js'
- './web/locales:/home/web/locales'
- './config:/home/config'
- './web/topogen.sh:/home/web/topogen.sh'
Copy link
Member

Choose a reason for hiding this comment

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

can you sort alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@corradio corradio requested a review from maxbellec March 6, 2018 06:38
});
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove one or both blank lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

topo2.arcs.forEach(arc=> {
topo1.arcs.push(arc);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

again, can you remove both blank lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -580,11 +596,44 @@ zoneFeatures = toListOfFeatures(zoneFeaturesInline);
// Write unsimplified list of geojson, without state merges
fs.writeFileSync('build/zonegeometries.json', zoneFeatures.map(JSON.stringify).join('\n'));

// Simplify
// Adds the zones from topo2 to topo1
function mergeTopoJsonSingleZone(topo1, topo2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move the function definition above the script part (to line ~550)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to moreDetails, zonesMoreDetails and topoMoreDetails that seems better for me as well.

@maxbellec
Copy link
Contributor

this is great! I'd also like to see a screenshot of Aruba Island. Could you also post of Acores or Canaries so that we can see the differences? It might be nice to have a bit more precision on a few island (but we might not see a big difference due to the quantization step).

Not a necessary change, I'm not a fan of the lessSimplify name. Though it's clear for someone that knows we're using topojson.simplify, maybe moreDetails would be more explicit? Anyway, keep the name you prefer.

@pascalheraud
Copy link
Contributor Author

Here is a screen shot of Aruba using moreDetails:true :
image

@pascalheraud
Copy link
Contributor Author

pascalheraud commented Mar 7, 2018

I tried to use moreDetails on Azores or Canaries but it produce the same geometries. As @maxbellec said, another operation may changes geometries.

- './web/src:/home/web/src'
- './web/locales:/home/web/locales'
Copy link
Member

Choose a reason for hiding this comment

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

Can you order alphabetically?

@corradio corradio merged commit 2d9a11a into electricitymaps:master Mar 8, 2018
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