Feature performance

[sjpfenninger]

Fixes #15

Summary of changes in this pull request

The main purpose of this PR was to address #15 and to optimise performance, but it bundles a couple of other changes too.

Claude Code was used for the memory and performance optimisations. In order to ensure these changes do not change results, a unit test suite checks against known-good data for a synthetic reference case, and for the Netherlands example already used in the integration test.

The following fixes and behaviour changes are also included:

• protected is now a fraction of a pixel (0-1), but can still be treated as a binary layer to keep the previous behaviour 1:1.
• The previous few PRs were not fully functional and are now fixed: scenario overrides were non-functional, ship-travel was looking for the non-scenario-aware top-level techs key, and the ship density downloads were failing unless forced to HTTP 1.1.

Performance improvements included:

• Protected areas processing: now only reads those features that intersect the supplied reference raster, and uses streaming in batches via OGR+GDAL rather than loading everything at once.
• Resampling: now reads inputs in windows and deals with arrays directly, with layers written to the NetCDF one at a time. To optimise memory use it uses float32 rather than float64. A land-cover lookup table is used instead of string replacement.
• Area potential calculations: build a "keep mask" iteratively which is then applied in one go, and also switch to float32 output.
• Reporting: strip-by-strip bincount aggregation reduces the need for keeping everything in memory at once, and the plotting is now memory-optimised and moved into separate rules as it proved to be particularly memory intensive.
• Snakemake-specific improvements: threads: is passed through to GDAL; resources: mem_mb allows constraining parallelisation on memory-limited machines, and the memory-intensive rules also write a benchmark file for further monitoring.

Testing added:

• New test-unit pixi environment pinned to the same versions as workflow/envs/module.*.pin.txt (sync enforced by a test)
• Reference data regeneration via pixi run update-reference; diffs to tests/reference/ are intentional and should be reviewed.

Reviewer checklist

• There are no pip dependencies in the module's environment files (workflow/envs/).
• All rules use pathvars (e.g., <results>) in their inputs and outputs.
• The integration test-suite is successful, including:
  • pre-commit.ci tests pass.
  • tests pass for all relevant OS configurations (linux, osx, windows).
• Module documentation is up-to-date, including:
  • INTERFACE.yaml mentions all relevant pathvars and wildcards.
  • README.md describes how to use the module and has the necessary citations.

[irm-codebase]

Looks good overall, but the unit tests need better environment isolation.

Details

In terms of memory efficiency this is night and day. Really impressive.
Some areas of the code seem a bit over optimised (e.g., plotting). It's ok to keep that as long as it does not become a maintenance burden.

General tests carried out:

• Executed the integration test.
• Executed for Mexico at state resolution (30+ shapes). Took ~13 minutes w/ 8 cores.

dask handles larger than memory operations well, as expected:

[irm-codebase]

consider renaming this to _plots.py or something similar, as everything here is related to plotting.

[irm-codebase]

I agree on using fig.savefig. Keeping bbox_inches="tight" might still be useful though, to avoid too much white space.

[sjpfenninger]

It seems that bbox_inches="tight" requires rendering the whole figure an additional time, so it does have quite an impact..

[irm-codebase]

Tests from here on seem a bit over engineered. We can keep, but we should not be overly attached to them.

[irm-codebase]

This PR fixes hard-blocking issues related to this new feature.
#28

[jnnr]

Thanks for this! I am done with a first review. Test runs are running at the moment - I will check if the old results are reproduced for a European setting. I looked at most relevant parts, except for scripts/area_potential.py and scripts/resample.py. I will inspect them in a second round.

One of my central concern among the comments is that the test oracles probably do not give the validity that we desire for unit tests. In my view, it would be better to establish a source of truth can be easily checked by a human.

[jnnr]

This oracle repeats the code implementing the logic of get_area_potential

module_area_potentials/workflow/scripts/area_potential.py

Line 18 in e6101a0

def get_area_potential(

Therefore, limited validity is provided.

A better approach would be to define and document expected results for simple examples that can be easily verified visually. For example, see https://github.com/modelblocks-org/gregor/blob/main/test/_files/test.png

[jnnr]

Not sure how independent from

module_area_potentials/workflow/scripts/resample.py

Line 73 in e6101a0

def land_cover_mask(ds_land_cover, mapped, category_id):

this really is, but fine for now.

[irm-codebase]

Looking good!