[tsmbland]

Description

Tidying up the investment graph module in preparation for #1557 and future work. Heavily aided by AI. My initial attempt at #1557 was a bit clunky, so I asked for ideas to tidy up the module, make these refactoring changes on the base branch, then rebase #1557 on top, therefore separating refactoring from the feature changes. I also told it some ideas about future changes, where I expect we'll switch the investment graph to have agents as nodes rather than market sets (just a vague idea at this point), and to keep this in the back of its mind.

Main changes:

• order_sccs has become order_scc - ordering one SCC rather than all of them, called in a loop. I think this makes the function a bit cleaner and removes a nesting level.
• removes init_investment_graph entirely. Graphs are now initialised with a single filter_map call. The current approach filter_maps, then copies nodes and edges to a new graph, which I think is a leftover from a time where at this point we were combining multiple region-level graphs, but no longer have to do this after Combine region graphs into single graph for each year #1569.
• We're also now deferring the point at which we convert (commodity, region) nodes to MarketSet until after cycle detection and ordering. Previously we made the switch before, which meant that we had to pass two different graphs to order_sccs (one with (commodity, region) nodes and one with MarketSet nodes). This is way cleaner now.
• Helper functions is_market and to_market_set
• Some helper functions in the tests for building test graphs.

I think this is undoubtedly an improvement.

Fixes # (issue)

Type of change

• Bug fix (non-breaking change to fix an issue)
• New feature (non-breaking change to add functionality)
• Refactoring (non-breaking, non-functional change to improve maintainability)
• Optimization (non-breaking change to speed up the code)
• Breaking change (whatever its nature)
• Documentation (improve or add documentation)

Key checklist

• All tests pass: $ cargo test
• The documentation builds and looks OK: $ cargo doc
• Update release notes for the latest release if this PR adds a new feature or fixes a bug
  present in the previous release

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

❌ Patch coverage is 91.62304% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.89%. Comparing base (60a012a) to head (cb2826f).

Files with missing lines	Patch %	Lines
src/graph/investment.rs	91.62%	6 Missing and 9 partials ⚠️
src/input.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1576      +/-   ##
==========================================
- Coverage   90.90%   90.89%   -0.02%     
==========================================
  Files          61       61              
  Lines        9116     9138      +22     
  Branches     9116     9138      +22     
==========================================
+ Hits         8287     8306      +19     
+ Misses        507      505       -2     
- Partials      322      327       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟢 Approval recommended

No active review issues are identified for the investment graph refactor.

No open issues 🎉

What changed in this PR

This PR simplifies the investment graph module, retaining the existing approach to ordering markets and grouping independent investment decisions.

Changes:

• Refactors graph filtering and cycle condensation to use commodity graph indices directly.
• Replaces order_sccs with a per-component order_scc helper and simplifies test setup and assertions.
• Updates documentation and a pricing comment to reference the renamed helper.

File	Description
src/​simulation/​prices.rs	Updates the helper name in a comment; no functional change.
src/​graph/​investment.rs	Simplifies investment graph construction, cycle ordering, and test helpers.
docs/​model/​commodity_graphs.md	Updates the cycle-ordering API reference and link.

_{Review effort: Balanced}

[alexdewar]

LGTM! Always nice to have a PR that removes code.

[alexdewar]

You could define a from_scc method for MarketSet instead. Up to you.

[alexdewar]

Just noting that there are a lot of Vecs of size n in this function, whereas you could have a smaller number (maybe just one) with a struct containing different fields in. I'm not saying you should do that, just that it's an option. The downside of manually working with indices is that code is more prone to off-by-one errors etc. (using iterators may be more idiomatic). That said, it's pretty clear as it stands (notwithstanding the fact that I don't actually understand the algorithm in any detail 😄).