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

Add depth parameter #46

Merged
merged 7 commits into from
Jun 9, 2022
Merged

Conversation

ClaroHenrique
Copy link
Contributor

@ClaroHenrique ClaroHenrique commented Jun 5, 2022

This PR aims to add the depth parameter in the GADM.get function.
This feature is mentioned in issue 40.

@juliohm
Copy link
Member

juliohm commented Jun 5, 2022

Thank you @ClaroHenrique for starting this PR ❤️ We need to replace the option children by the option level. When level=0 that is equivalent to children=false, when level=1 that is equivalent to children=true and when level=2,3,... that is a new feature with a single table containing all subtables merged into a single one.

Please let me know if something is not clear. I can provide a simple example with states and cities in Brazil.

@ClaroHenrique
Copy link
Contributor Author

ClaroHenrique commented Jun 5, 2022

Hi @juliohm, I think i get it now.
My main question is about how can we join the subtables in a single one.
In the original database, the data is separated by level in tables with different columns.

Schema example

image

Is there a problem if the resulting single table hold all those columns?

@juliohm
Copy link
Member

juliohm commented Jun 5, 2022

Hi @ClaroHenrique can you provide an example of result with Brazil data? What are the tables we get when we set level=0,1,2?

We can then try to figure out a combination of columns that makes sense in general.

@ClaroHenrique
Copy link
Contributor Author

@juliohm, those are the results from the database downloaded in https://data.biogeo.ucdavis.edu/data/gadm3.6/gpkg/gadm36_BRA_gpkg.zip.

Level 0

image

Level 1

image

Level 2

image

@juliohm
Copy link
Member

juliohm commented Jun 5, 2022

Nice, so the idea is that if a user types

get("BRA", "Alagoas", level=1)

we return all the cities of Alagoas in a single table. This consists of filtering the rows of the table of level 2 that have Alagoas as the state. More generally, we want

get(country, state, city, municipality, ..., level=n)

to return all the leaves of the node country->state->city->municipality that are n steps deeper in the tree.

This could be implemented with different algorithms. We could use a depth-first-search starting from the municipality to find the children at depth n. We then push these tables to a list of tables to be merged. At the end all these tables will be at the same level, so we should expect them to have the same columns. If they don't have the same columns, we should probably investigate the issue further.

I can take a closer look at the dataset if it is still not clear what the end goal is.

@ClaroHenrique
Copy link
Contributor Author

ClaroHenrique commented Jun 5, 2022

It's fine. I just missunderstood this message.
For a moment I thought that we were going to process multiple levels in one get.

@ClaroHenrique ClaroHenrique changed the title Add level parameter (WIP) Add depth parameter (WIP) Jun 8, 2022
@ClaroHenrique ClaroHenrique marked this pull request as ready for review June 8, 2022 02:46
@ClaroHenrique ClaroHenrique changed the title Add depth parameter (WIP) Add depth parameter Jun 8, 2022
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

@ClaroHenrique this is an amazing PR. Thank you for improving the API, the code is very elegant.

src/GADM.jl Outdated Show resolved Hide resolved
test/GADM.jl Outdated Show resolved Hide resolved
ClaroHenrique and others added 2 commits June 9, 2022 12:59
@juliohm juliohm merged commit c710c00 into JuliaGeo:master Jun 9, 2022
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.

2 participants