Rename investmentset marketset by dc2917 · Pull Request #1371 · EnergySystemsModellingLab/MUSE2

dc2917 · 2026-06-24T15:02:26Z

Description

This PR:

Moves InvestmentSet and associated functions out of the investment module into a new market module (with some (un)educated guesses at which functions should be moved, and which functions should remain but be made public))
Renames InvestmentSet to MarketSet
Updates graph::investment to reflect these changes (also with some guesswork at which terminology should be updated)

Fixes #1337

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 · 2026-06-24T15:07:22Z

Codecov Report

❌ Patch coverage is 94.94048% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (76db0a2) to head (9906692).
⚠️ Report is 45 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/market.rs	94.73%	13 Missing and 3 partials ⚠️
src/simulation/investment.rs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1371      +/-   ##
==========================================
- Coverage   89.80%   89.68%   -0.12%     
==========================================
  Files          58       59       +1     
  Lines        8524     8406     -118     
  Branches     8524     8406     -118     
==========================================
- Hits         7655     7539     -116     
+ Misses        556      550       -6     
- Partials      313      317       +4

☔ 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.

tsmbland

On board with the name change. I think some of the stuff moved over from simulation::investment to simulation::market should probably still belong in simulation::investment, particularly the select_assets functions, and possibly even get_demand_portion_for_market and get_responsible_agents. That get's a bit messy since select_assets is a method of MarketSet. I think you can define methods away from where the struct lives, but maybe that's messy, and maybe select_assets would be better as a standalone function anyway?

@alexdewar what do you think?

alexdewar

Looking good! I've suggested a few tweaks

alexdewar · 2026-06-25T08:12:31Z

 /// agent's portion of the commodity demand and the number of years elapsed since the previous
 /// milestone year).
-fn calculate_investment_limits_for_candidates(
+pub fn calculate_investment_limits_for_candidates(


This function and get_asset_options are only used in market.rs now, so they could be left private and moved there.

select_best_assets is also only used in market.rs, but given that that's the heart of the investment-related code, I feel like it fits better in investment.rs.

Do you agree @tsmbland?

I do think all the core investment-related code should be in investment.rs, and leave market.rs for generic market-related stuff like the iter and display implementations

I've given things a bit of a shuffle, trying to accommodate both of your suggestions 😅

I've tried to use a bit of logic based on how/where the functions are used and how they're described in their docstrings (and names), but I obviously have no personal feelings on the matter so am happy to move things as you see fit.

alexdewar · 2026-06-25T08:14:57Z

+    /// Assets are selected for a single market using `select_assets_for_single_market`
+    Single((CommodityID, RegionID)),
+    /// Assets are selected for a group of markets which forms a cycle.
+    /// Experimental: handled by `select_assets_for_cycle` and guarded by the broken options
+    /// parameter.
+    Cycle(Vec<(CommodityID, RegionID)>),
+    /// Assets are selected for a layer of independent `MarketSet`s


These identifiers can be turned into links for the docs:

Suggested change

/// Assets are selected for a single market using `select_assets_for_single_market`

Single((CommodityID, RegionID)),

/// Assets are selected for a group of markets which forms a cycle.

/// Experimental: handled by `select_assets_for_cycle` and guarded by the broken options

/// parameter.

Cycle(Vec<(CommodityID, RegionID)>),

/// Assets are selected for a layer of independent `MarketSet`s

/// Assets are selected for a single market using [`select_assets_for_single_market`]

Single((CommodityID, RegionID)),

/// Assets are selected for a group of markets which forms a cycle.

/// Experimental: handled by [`select_assets_for_cycle`]. May not work in every case.

Cycle(Vec<(CommodityID, RegionID)>),

/// Assets are selected for a layer of independent [`MarketSet`]s

Edit: Also changed the text for Cycle as it's no longer a hidden option.

@alexdewar there may be a reason why these weren't linked before:

Generating API documentation Documenting muse2 v2.1.0 (/home/runner/work/MUSE2/MUSE2) error: public documentation for `Single` links to private item `select_assets_for_single_market` Error: --> src/simulation/market.rs:26:57 | 26 | /// Assets are selected for a single market using [`select_assets_for_single_market`] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this item is private | = note: this link resolves only because you passed `--document-private-items`, but will break without = note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(rustdoc::private_intra_doc_links)]` error: public documentation for `Cycle` links to private item `select_assets_for_cycle` Error: --> src/simulation/market.rs:29:36 | 29 | /// Experimental: handled by [`select_assets_for_cycle`]. May not work in every case. | ^^^^^^^^^^^^^^^^^^^^^^^ this item is private | = note: this link resolves only because you passed `--document-private-items`, but will break without

alexdewar · 2026-06-25T08:55:20Z

On board with the name change. I think some of the stuff moved over from simulation::investment to simulation::market should probably still belong in simulation::investment, particularly the select_assets functions, and possibly even get_demand_portion_for_market and get_responsible_agents. That get's a bit messy since select_assets is a method of MarketSet. I think you can define methods away from where the struct lives, but maybe that's messy, and maybe select_assets would be better as a standalone function anyway?

@alexdewar what do you think?

Ha, I just messaged you suggesting kind of the opposite (moving more things to market.rs) and asking what you think. I can see it both ways, but I feel like getting the set of assets we might invest in isn't "core" investment code (there's no investment) and given that there's quite a lot of this code, maybe it would be better somewhere else. investment.rs is quite long, so it would be nice to trim it down a bit.

Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>

alexdewar

I'd be happy with this, but I thought we settled on moving more stuff out of market.rs, so it was pretty much just MarketSet? Could make the MarketSet::select_assets a regular function and put it in investment.rs.

I don't mind merging as is though -- investment.rs is quite long as it is. What do you think @tsmbland?

In terms of the cargo doc errors, I think we should fix it by making the functions pub. It makes sense that it would complain about private things being referred to in doc comments for public things as cargo doc doesn't document private items by default.

tsmbland · 2026-06-26T14:08:42Z

I'd be happy with this, but I thought we settled on moving more stuff out of market.rs, so it was pretty much just MarketSet? Could make the MarketSet::select_assets a regular function and put it in investment.rs.

I don't mind merging as is though -- investment.rs is quite long as it is. What do you think @tsmbland?

My vote would be to keep this in investment.rs, and then break that into submodules. I'm not that fussed though, won't veto this!

alexdewar · 2026-06-26T14:16:14Z

I'd be happy with this, but I thought we settled on moving more stuff out of market.rs, so it was pretty much just MarketSet? Could make the MarketSet::select_assets a regular function and put it in investment.rs.
I don't mind merging as is though -- investment.rs is quite long as it is. What do you think @tsmbland?

My vote would be to keep this in investment.rs, and then break that into submodules. I'm not that fussed though, won't veto this!

How about we merge this for now (once cargo doc is happy) as we're going to take another pass at the investment/appraisal code soon anyway (#1378)?

…tions public

dc2917 · 2026-06-26T17:01:50Z

How about we merge this for now (once cargo doc is happy) as we're going to take another pass at the investment/appraisal code soon anyway (#1378)?

Sounds good to me.

9906692 fixes the cargo doc issue now those functions are public, so will re-request review.

dc2917 added 3 commits June 24, 2026 15:42

Move InvestmentSet to a new markets module

ede02d9

Rename InvestmentSet -> MarketSet

7d3ed03

Rebrand to reflect InvestmentSet -> MarketSet

e4a33c7

dc2917 requested review from alexdewar and tsmbland June 24, 2026 15:02

tsmbland reviewed Jun 25, 2026

View reviewed changes

alexdewar requested changes Jun 25, 2026

View reviewed changes

dc2917 and others added 3 commits June 25, 2026 11:26

Turm identifiers into links for the docs

54ceb49

Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>

Tidy imports and improve investment-market separation of concerns

0c837fc

Replace 'investment set' -> 'market set'

eb225bd

dc2917 requested review from alexdewar and tsmbland June 25, 2026 16:45

alexdewar reviewed Jun 26, 2026

View reviewed changes

Make select_assets_for_single_market and select_assets_for_cycle func…

9906692

…tions public

dc2917 requested a review from alexdewar June 26, 2026 17:02

Uh oh!

Conversation

dc2917 commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexdewar Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

dc2917 Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dc2917 Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

tsmbland commented Jun 26, 2026

Uh oh!

alexdewar commented Jun 26, 2026

Uh oh!

dc2917 commented Jun 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dc2917 commented Jun 24, 2026 •

edited

Loading

codecov Bot commented Jun 24, 2026 •

edited

Loading

alexdewar Jun 25, 2026 •

edited

Loading

alexdewar commented Jun 25, 2026 •

edited

Loading