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

Spain provinces and autonomous communities #146

Merged
merged 10 commits into from
Dec 11, 2019

Conversation

jsanz
Copy link
Member

@jsanz jsanz commented Nov 26, 2019

fixes #142

This PR splits Spanish provinces and autonomous communities (something similar to USA counties and states) into two separated layers.

As described in #142, a few wikidata entries had two different ISO codes, one for the province and another for the autonomous community.

To retrieve the data for the provinces, since relations in OSM were properly modeled (after adding a few Wikidata tags), the SPARQL query is quite different from the typical ones we've been using.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@jsanz jsanz requested a review from nickpeihl November 26, 2019 14:06
@jsanz jsanz self-assigned this Nov 26, 2019
@jsanz
Copy link
Member Author

jsanz commented Nov 26, 2019

@nickpeihl please take a look when you have some time and let me know what you think

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

The Wikidata ids in the provinces layer have been demoted to feature properties.id rather than the feature.id. We do have an open issue for adding wikidata ids to the properties. But I think that should be done all at once in a separate PR.

Is it possible the fixgeometries.py script caused the demotion? As I recall, QGIS does not respect the feature.id property in GeoJSON files.

Also, we need to update the versions property in sources/es/provinces.hjson to be something like 1 - 7 (see node-semver docs).

sources/es/autonomous-communities.hjson Outdated Show resolved Hide resolved
query: {
sparql:
'''
#defaultView:MapRegions
Copy link
Member

Choose a reason for hiding this comment

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

perhaps fix the indentation here? not a big deal, but just looks nicer.

Co-Authored-By: Nick Peihl <nickpeihl@gmail.com>
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@elasticmachine
Copy link
Collaborator

💔 Build Failed

@jsanz
Copy link
Member Author

jsanz commented Nov 27, 2019

@nickpeihl thanks for your comments 🙌

I've moved the id's to the feature level using the script below. Should I put it in the scripts folder?

Also, should this check be included somehow in the test suite? I don't know exactly how we check geometries on the test process, but it would be great if we could ensure the ids are correctly placed.

Script to move id's
/*
 * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
 * or more contributor license agreements. Licensed under the Elastic License;
 * you may not use this file except in compliance with the Elastic License.
 */

const fs = require('fs');

try {
  const filePath = process.argv[2];

  if (!filePath) {
    throw new Error(`Add the GeoJSON file path to process. e.g. node scripts/move-ids.js data/usa_states_v1.geo.json`);
  }

  const file = fs.readFileSync(filePath, 'utf8');
  const geojson = JSON.parse(file);

  if (geojson.features[0].id) {
    throw new Error('File already has ID\'s at feature level, nothing to do');
  }

  // overwrite the features moving the ID field at the feature level
  geojson.features = geojson.features.map(({ type, geometry, properties }) => {
    const { id, ...propsWithoutId } = properties;
    return { id, type, geometry, properties: propsWithoutId };
  })

  process.stdout.write(JSON.stringify(geojson))
} catch (error) {
  console.log(error.message)
  process.exit(1);
}

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@jsanz jsanz mentioned this pull request Nov 27, 2019
@nickpeihl nickpeihl self-requested a review December 3, 2019 19:38
@nickpeihl
Copy link
Member

Also, should this check be included somehow in the test suite? I don't know exactly how we check geometries on the test process, but it would be great if we could ensure the ids are correctly placed.

I looked at our existing data and having a top-level id property for each feature is not a hard constraint. For example, South Korea municipalities do not have top level feature ids because it comes from a different data source. Most of our other data is generated from SPARQL which automatically creates the id property.

So I am going to backtrack a bit here. Sorry, but now I do not think we need to require every feature have a top-level id property. But maybe we should constrain feature properties to only those specified in the source metadata (essentially the reverse of this issue)?

This PR lgtm! Let's merge this and consider the other questions separately.

@jsanz jsanz merged commit ff3a7b1 into elastic:feature-layers Dec 11, 2019
@jsanz jsanz deleted the feature-layers-es branch December 11, 2019 18:08
@jsanz
Copy link
Member Author

jsanz commented Dec 11, 2019

So I am going to backtrack a bit here. Sorry, but now I do not think we need to require every feature have a top-level id property. But maybe we should constrain feature properties to only those specified in the source metadata (essentially the reverse of this issue)?

Shall we open a new issue to track this? This should be placed in the testing logic right?

@nickpeihl
Copy link
Member

Yes, this is would be in the testing logic. I have created an issue to track this. Thanks.

@nickpeihl nickpeihl mentioned this pull request Oct 20, 2021
5 tasks
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