Refactor equal metrics comparison code

[alexdewar]

Description

If MUSE2 has to choose between investing in two equally good assets, it prefers commissioned over non-commissioned assets and newer over old.

The logic for this currently lives in appraisal.rs and is done as part of AppraisalOutput::compare_metric. As discussed in #1027, I think this code for the fallback comparison would be better elsewhere, partly because it doesn't actually have anything to do with metrics and partly because it'll make refactoring more difficult.

I've added a new function (compare_assets_fallback) to investment.rs with tests. While I was at it, I noticed the code for warning users when there are equally good assets could be simplified, so I did that too. I've changed it from a warning-level message to debug, as it isn't something most users will care about (it doesn't indicate a problem) and it'll also be noisy in the case of divided assets (see discussion on #1030).

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

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 92.95775% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.75%. Comparing base (5d8a60d) to head (c632d74).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/investment.rs	90.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1039      +/-   ##
==========================================
+ Coverage   81.20%   81.75%   +0.54%     
==========================================
  Files          53       53              
  Lines        7269     7263       -6     
  Branches     7269     7263       -6     
==========================================
+ Hits         5903     5938      +35     
+ Misses       1071     1030      -41     
  Partials      295      295              

☔ View full report in Codecov by Sentry.
📢 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]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[dalonsoa]

I've a change in the docstring, because it's a bit confusing, but it looks good, otherwise.

[tsmbland]

Looks good! Agree this gives better separation of concerns.

Perhaps we could include the commission year in the debug message, just so it's clear when it's selecting newer assets over older assets? (although I guess you can figure this out from the asset id)

[alexdewar]

Looks good! Agree this gives better separation of concerns.

Perhaps we could include the commission year in the debug message, just so it's clear when it's selecting newer assets over older assets? (although I guess you can figure this out from the asset id)

Good idea. Done.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The logic using tuple_windows().take_while() doesn't include the last element in a sequence of equal outputs. For example, if outputs[0], outputs[1], and outputs[2] are all equal, only outputs[0] and outputs[1] will be included in asset_details, missing outputs[2]. This makes the warning message incomplete.

Consider adding the final equal element after the iterator chain, or use a different approach such as: outputs.iter().skip(1).take_while(|b| outputs[0].compare_metric(b).is_eq()) and then prepend outputs[0] to the results.

[alexdewar]

Also a good spot... I'll rework it.

[Copilot]

The new implementation changes behavior when comparing two non-commissioned (candidate) assets. The old compare_with_equal_metrics returned Ordering::Equal when both assets were non-commissioned, regardless of their commission years. The new tuple comparison (asset2.is_commissioned(), asset2.commission_year()).cmp(&(asset1.is_commissioned(), asset1.commission_year())) will order non-commissioned assets by commission year.

This means candidates with different years will now be ordered (newer first) instead of being treated as equal. This appears to be unintentional since the PR is described as a refactoring with no functional changes. If this behavior change is intentional, it should be documented in the PR description and tested.

Suggested change

	(asset2.is_commissioned(), asset2.commission_year())
	.cmp(&(asset1.is_commissioned(), asset1.commission_year()))
	// Preserve legacy behaviour: if both assets are uncommissioned (candidates),
	// treat them as equal regardless of their commission years.
	if !asset1.is_commissioned() && !asset2.is_commissioned() {
	Ordering::Equal
	} else {
	(asset2.is_commissioned(), asset2.commission_year())
	.cmp(&(asset1.is_commissioned(), asset1.commission_year()))
	}

[alexdewar]

Oh, this is a good spot! I hadn't clocked that this was a functional change, although presumably the new behaviour was what was intended originally!