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 option to create subregion with minimalistic impact to the workflow #1300

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

virio-andreyana
Copy link
Contributor

@virio-andreyana virio-andreyana commented Jan 16, 2025

Closes #788 and #1046.

Changes proposed in this Pull Request

This is a new approach to #1046 that allows countries to be redefined into subregions and back. It's more robust because it does not impact any other rules other that the one that is being focused, namely in simplify network:

Before:
image

After:
image

Again, this requires config such as this:

subregion: # remove 'false' if subregion are to be specified
  define_by_gadm:
    Java-Bali/ID:
      ID: [2,4,7,9,10,11,33]
    Peninsular/MY:
      MY: [1,2,3,4,6,7,8,9,10,11,12,15,16]
    Papua/ID:
      ID: [22,23]
    Sarawak/MY:
      MY: [14]
    Maluku/ID:
      ID: [18,19]
    Sulawesi/ID:
      ID: [6,25,26,27,28,29]
    Sabah/MY:
      MY: [5,13]
    Kalimantan/ID:
      ID: [12,13,14,34,35]
    Nusa-Tenggara/ID:
      ID: [20,21]
    Sumatra/ID:
      ID: [1,3,5,8,16,17,24,30,31,32]

As suggested by @davide-f, we also have the option to use a custom shapes for this

subregion: # remove 'false' if subregion are to be specified
  define_by_gadm: false
  path_custom_shapes: "../custom_shapes.geojson"

Checklist

  • I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

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

Hello @virio-andreyana :D

Many thanks for the revision! quite neat and very interesting!

I've added some comments, good to align.
On top of them, please go through the checklist, I believe it would be great to:

  • add a release note
  • add changes into the configuration options [the table doc/configtables/*.csv and line references in doc/configuration.rst and doc/tutorial.rst].
  • It would be advisable to integrate in build_shapes docstring (at the top) a description of the output including your proposal
  • Include the feature in the tests. It could be added to the landlock test to avoid adding a new test config file
  • Add the configuration option into config.default.yaml

The configuration file may contain a flag enable to be used as flag on whether the feature is used or not, instead of the option being present or not.
By doing so, the template of the config option can be added to config.default.yaml

What do you think?

This PR is architectural; any additional review is welcome

Snakefile Outdated Show resolved Hide resolved
Snakefile Outdated Show resolved Hide resolved
Snakefile Outdated Show resolved Hide resolved
scripts/build_shapes.py Show resolved Hide resolved
scripts/build_shapes.py Show resolved Hide resolved
scripts/build_shapes.py Outdated Show resolved Hide resolved
scripts/build_shapes.py Outdated Show resolved Hide resolved
@virio-andreyana
Copy link
Contributor Author

I've now added the documentations and test runs in the land-lock config for Botswana. It looks like the GADM formatting from the premade databundle for BW is somewhat outdated. But that is fixed by hashing it out, and letting the GADM be downloaded during build_shapes.

The last problem is this def save_to_geojson() function. We have three definitions for the same function: in _helper.py, build_shapes.py, and cluster_network.py. I want to make an empty geojson based on build_shapes.py definition by having an empty lis as its input, but the program pulls out _helper.py definition, which requires the input to be at least a dataframe. I couldn't find a solution that suffice them both. So what do you think is the correct approach @davide-f :

  1. (simple) rename the def save_to_geojson() to be different in each script
  2. (difficult) create def save_to_geojson() in _helper.py that is valid for all conditions

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

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

I've now added the documentations and test runs in the land-lock config for Botswana. It looks like the GADM formatting from the premade databundle for BW is somewhat outdated. But that is fixed by hashing it out, and letting the GADM be downloaded during build_shapes.

The last problem is this def save_to_geojson() function. We have three definitions for the same function: in _helper.py, build_shapes.py, and cluster_network.py. I want to make an empty geojson based on build_shapes.py definition by having an empty lis as its input, but the program pulls out _helper.py definition, which requires the input to be at least a dataframe. I couldn't find a solution that suffice them both. So what do you think is the correct approach @davide-f :

  1. (simple) rename the def save_to_geojson() to be different in each script
  2. (difficult) create def save_to_geojson() in _helper.py that is valid for all conditions

Thanks! Starting from save_to_geojson, your PR seems a bit old, please update it as this issue was solved [or improved].
We now have a generalized formulation in _helpers

Can you elaborate on the preloaded data of GADM of botswana?
Commenting the line in bundle config should not actually avoid downloading the file of the bundle it is just for information.
What issue have you found about it?

P.S. After updating the branch, please, run pre-commit run --all; the precommit is not passing

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.

Support automatic clustering of regions or districts within a country using gadm
2 participants