New Structure, regions & parallelization - #15
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures InSEEDS “models” into “realisations”, introduces a country/region (3-level) hierarchy intended to enable country-level parallelization, and updates tests + CI to match the new structure and output collection approach.
Changes:
- Added new
regenerative_tillage_regionsrealisation (Model/main/config) with country-level entities and parallelization settings. - Migrated
regenerative_tillageto the newrealisations/structure and adjusted model update/output collection flow. - Expanded/rewrote tests + pytest fixtures, and enhanced CI workflows (coverage, build checks) plus a new release workflow.
Reviewed changes
Copilot reviewed 44 out of 48 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
inseeds/realisations/regenerative_tillage_regions/model.py |
New regions-aware model wiring (World/Country/Cell/Farmer) and per-year update flow |
inseeds/realisations/regenerative_tillage_regions/main.py |
New entry point using pycopanlpjml.run.run_simulation |
inseeds/realisations/regenerative_tillage_regions/config.yaml |
Adds parallelization/output configuration for regions run |
inseeds/realisations/regenerative_tillage/model.py |
Updates model to use lpjml.Model, new init/update/output collection behavior |
inseeds/realisations/regenerative_tillage/main.py |
Updates imports to the new realisations location |
inseeds/components/farming/* |
Adds Region/Country mixins, updates World.update, refactors tillage farmer base class usage |
inseeds/components/base/* |
Removes local output-writing logic; migrates to pycopanlpjml output mixins/classes |
tests/conftest.py |
Reworks fixtures to cache LPJmL pickles and create/run models once per session |
tests/test_regenerative_tillage.py |
Updates tests for new structure/output comparison approach |
tests/test_regenerative_tillage_regions.py |
New test suite for 3-level hierarchy and compatibility checks |
tests/test_components_base.py / tests/test_components_farming.py / tests/test_main.py |
Adds unit/import/entry-point tests |
.github/workflows/check.yml |
Updates CI: newer setup-python, coverage scope, build + twine checks |
.github/workflows/release.yml |
Adds automated release pipeline (tests, lint, build, PyPI publish, GH release) |
CITATION.cff |
Updates cited version metadata |
README.md |
Updates documentation to refer to realisations/ structure |
scripts/* and tests/data/write_testdata.py |
Updates script paths and LPJmL run helper usage for new layout |
Comments suppressed due to low confidence (1)
inseeds/realisations/regenerative_tillage/model.py:121
has_cropsis an xarray DataArray comparison, soif not has_crops:will raise (DataArray has no truth value). Convert to a Python bool/scalar first (e.g., use.item()/.values.item()after the reduction) before using it in theifcondition.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 'mpi' = reuse LPJmL's MPI processes (recommended if LPJmL uses MPI) | ||
| # 'serial' = force serial execution | ||
| mode: 'dask' # 'auto' | ||
|
|
There was a problem hiding this comment.
This config hard-codes parallelization mode: 'dask'. If Dask isn't installed/available in the target environment, runs will fail even though auto/serial would work. Consider defaulting to auto (as in the non-regions config) and documenting how to opt into Dask explicitly.
| type: software | ||
| title: 'Model of integrated social-ecological resilient land systems (InSEEDS)' | ||
| version: 0.2.8 | ||
| version: 1.2.0 |
There was a problem hiding this comment.
pyproject.toml still declares the package version as 0.2.8, but CITATION.cff is updated to 1.2.0. Align these versions (or document why they intentionally differ) so packaging, releases, and citation metadata stay consistent.
| version: 1.2.0 | |
| version: 0.2.8 |
| self.countries = [] | ||
| print( | ||
| "Warning: No country data available. " | ||
| "Running without country-level structure." | ||
| ) |
There was a problem hiding this comment.
Library code prints a warning directly during model initialization. This is hard to control for downstream users and test runners; prefer warnings.warn(...) or the project logger so callers can filter/redirect it.
| # @property | ||
| # def farmers(self): | ||
| # """Return the set of all farmers.""" | ||
| # farmers = { | ||
| # farmer | ||
| # for farmer in self.individuals | ||
| # if farmer.__class__.__name__ == "Farmer" # noqa | ||
| # } | ||
| # return farmers | ||
|
|
||
| @property | ||
| def farmer(self): | ||
| """Return the first farmer.""" | ||
| farmers = self.farmers | ||
| if len(farmers) == 0: | ||
| return None | ||
| return list(farmers)[0] | ||
| # @property | ||
| # def farmer(self): | ||
| # """Return the first farmer.""" | ||
| # farmers = self.farmers | ||
| # if len(farmers) == 0: | ||
| # return None | ||
| # return list(farmers)[0] |
There was a problem hiding this comment.
This file comments out the farmers/farmer properties rather than removing them or keeping them implemented. Consider deleting the dead commented code (or reintroducing the properties if still part of the public API) to keep the module clean and avoid confusion.
| has_crops = cell.from_earth.cftfrac.sum("band") > 0 | ||
| if not has_crops: | ||
| continue |
There was a problem hiding this comment.
has_crops is an xarray DataArray comparison, so if not has_crops: will raise (DataArray has no truth value). Convert to a Python bool/scalar first (e.g., use .item() / .values.item() after the reduction) before using it in the if condition.
| build: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
This workflow grants contents: write, but it only checks out code, runs tests/lint/build, and uploads coverage. Reduce to least-privilege (typically contents: read) to avoid unnecessarily broad repository write permissions for PRs.
| contents: write | |
| contents: read |
Implementation of regions (countries) into InSEEDS and parallelization based on countries
... for more information
Structural changes in InSEEDS structure: