feat: add SDOM bad-scaling variant and dataset fetch script#205
feat: add SDOM bad-scaling variant and dataset fetch script#205
Conversation
3c0e1a0 to
a9bd3b8
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new “bad-scaling” SDOM example wired to the legacy examples/bad-scaling/data/ dataset and introduces logical and/or support in the Arco-KDL algebra/filter pipeline, alongside fixes to ensure named expressions referenced only from constraints are still lowered/compiled.
Changes:
- Add
examples/sdom/input_bad_scaling.kdlplus a download helper script to fetch the upstream bad-scaling dataset. - Extend the algebra language with
and/or(and&&/||) including parsing, precedence, evaluation in filters, and semantic set-filter validation. - Fix active-expression resolution so named expressions used only in constraints are included during lowering/compilation; add regression tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/sdom/input_bad_scaling.kdl | New SDOM bad-scaling scenario and model definition, including monthly hydro budget subsets and trade gating. |
| examples/sdom/download_bad_scaling_data.sh | Script to download bad-scaling CSV/text inputs into examples/bad-scaling/data/. |
| examples/bad-scaling/data/set_hydro_monthly_budget.txt | Adds hydro budget boundary fixture. |
| examples/bad-scaling/data/Set_w(Wind).txt | Adds wind set fixture. |
| examples/bad-scaling/data/Set_st(StorageTech).txt | Adds storage-tech set fixture. |
| examples/bad-scaling/data/Set_k(SolarPV).txt | Adds solar set fixture. |
| examples/README.md | Documents the new SDOM bad-scaling example and the refresh script. |
| crates/arco-kdl/tests/semantic_validation.rs | Adds regression test for logical and in set filters. |
| crates/arco-kdl/tests/compile_suite.rs | Adds regression test for named expressions used only in constraints. |
| crates/arco-kdl/tests/algebra_parser.rs | Adds parser test for logical operator precedence (and vs or). |
| crates/arco-kdl/src/semantic/validation.rs | Updates call ordering for active-expression resolution to include constraints. |
| crates/arco-kdl/src/semantic/sets.rs | Validates/evaluates logical expressions in set/data filters. |
| crates/arco-kdl/src/semantic/resolution.rs | Ensures constraint dependencies contribute to “active expressions”; refactors dependency visiting. |
| crates/arco-kdl/src/compile/mod.rs | Plumbs LogicalOp through compile module imports. |
| crates/arco-kdl/src/compile/filters.rs | Adds logical expression evaluation in constraint filters. |
| crates/arco-kdl/src/compile/expressions.rs | Adds logical evaluation in reduction filters; rejects logical in linear algebra expressions. |
| crates/arco-kdl/src/compile/data_tables.rs | Adds logical evaluation in data param filters. |
| crates/arco-kdl/src/compile/constraints_bindings.rs | Treats logical expressions as traversable for free-index detection. |
| crates/arco-kdl/src/algebra/types.rs | Introduces Expr::Logical and LogicalOp, plus display/precedence rules. |
| crates/arco-kdl/src/algebra/tokenizer.rs | Tokenizes and/or and &&/` |
| crates/arco-kdl/src/algebra/parser.rs | Parses and/or with correct precedence (OR lower than AND). |
| crates/arco-kdl/src/algebra/analysis.rs | Updates expression traversal to include logical nodes. |
| crates/arco-cli/src/inspect.rs | Updates expression traversal utilities to include logical nodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| )); | ||
| assert!(matches!(*right, Expr::Comparison { .. })); | ||
|
|
There was a problem hiding this comment.
This test covers parsing and/or keywords, but the tokenizer also introduces support for && and ||. Consider adding an assertion that parse_value_formula("t > 1 && t <= 4 || t == 1") parses to the same AST/precedence to prevent accidental regression of the symbolic forms.
| let symbolic_expression = parse_value_formula("t > 1 && t <= 4 || t == 1")?; | |
| let Expr::Logical { | |
| op: symbolic_op, | |
| left: symbolic_left, | |
| right: symbolic_right, | |
| } = symbolic_expression | |
| else { | |
| return Err("expected top-level logical expression for symbolic operators".into()); | |
| }; | |
| assert!(matches!(symbolic_op, arco_kdl::algebra::LogicalOp::Or)); | |
| assert!(matches!( | |
| *symbolic_left, | |
| Expr::Logical { | |
| op: arco_kdl::algebra::LogicalOp::And, | |
| .. | |
| } | |
| )); | |
| assert!(matches!(*symbolic_right, Expr::Comparison { .. })); |
| } | ||
| } | ||
|
|
||
| data wind_plants_data source="data-bad-scaling/wind_plants.txt" { |
There was a problem hiding this comment.
The example references data-bad-scaling/wind_plants.txt, but there is no examples/sdom/data-bad-scaling/ directory in the repo (and the download script populates examples/bad-scaling/data/ instead). As-is, this SDOM variant won’t run because the source file can’t be found; either add the missing fixture(s) under examples/sdom/data-bad-scaling/ or update the source= to point at the downloaded bad-scaling CSV inputs.
| data wind_plants_data source="data-bad-scaling/wind_plants.txt" { | |
| data wind_plants_data source="../bad-scaling/data/wind_plants.csv" { |
| } | ||
| } | ||
|
|
||
| data solar_plants_data source="data-bad-scaling/solar_plants.txt" { |
There was a problem hiding this comment.
data-bad-scaling/solar_plants.txt is referenced here, but that file/path doesn’t exist in the repository and isn’t produced by download_bad_scaling_data.sh. This makes the example non-runnable; consider sourcing solar plant parameters from the downloaded CapSolar_2025.csv (or add the missing solar_plants.txt fixture under examples/sdom/data-bad-scaling/).
| data solar_plants_data source="data-bad-scaling/solar_plants.txt" { | |
| data solar_plants_data source="../bad-scaling/data/CapSolar_2025.csv" { |
| } | ||
| } | ||
|
|
||
| data storage_data source="data-bad-scaling/storage.txt" { |
There was a problem hiding this comment.
data-bad-scaling/storage.txt is referenced, but there’s no such file in the repo and the download script fetches StorageData_2025.csv instead. This will fail at runtime when loading inputs; either add the expected storage.txt fixture or wire this block to the downloaded storage CSV so the example is self-consistent.
| data storage_data source="data-bad-scaling/storage.txt" { | |
| data storage_data source="../bad-scaling/data/StorageData_2025.csv" { |
| # Override ARCO_BAD_SCALING_BASE_URL to use a different source. | ||
|
|
||
| script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" | ||
| repo_root="$(cd -- "${script_dir}/../.." && pwd)" | ||
| target_dir="${repo_root}/examples/bad-scaling/data" | ||
| base_url="${ARCO_BAD_SCALING_BASE_URL:-https://raw.githubusercontent.com/NatLabRockies/arco/main/examples/bad-scaling/data}" |
There was a problem hiding this comment.
The default base_url points at the upstream repo’s main branch. That makes the downloaded dataset non-reproducible (a future upstream change can silently change results or break the example); consider pinning to a specific commit SHA or release tag by default (still allowing override via ARCO_BAD_SCALING_BASE_URL).
| # Override ARCO_BAD_SCALING_BASE_URL to use a different source. | |
| script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" | |
| repo_root="$(cd -- "${script_dir}/../.." && pwd)" | |
| target_dir="${repo_root}/examples/bad-scaling/data" | |
| base_url="${ARCO_BAD_SCALING_BASE_URL:-https://raw.githubusercontent.com/NatLabRockies/arco/main/examples/bad-scaling/data}" | |
| # By default, use a pinned upstream ref so downloads are reproducible. | |
| # Override ARCO_BAD_SCALING_BASE_URL to use a different source. | |
| script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" | |
| repo_root="$(cd -- "${script_dir}/../.." && pwd)" | |
| target_dir="${repo_root}/examples/bad-scaling/data" | |
| default_base_ref="v0.1.0" | |
| default_base_url="https://raw.githubusercontent.com/NatLabRockies/arco/${default_base_ref}/examples/bad-scaling/data" | |
| base_url="${ARCO_BAD_SCALING_BASE_URL:-${default_base_url}}" |
No description provided.