Skip to content

Precompute investment objective coefficients#888

Merged
tsmbland merged 7 commits into
mainfrom
precompute_coefficients
Oct 9, 2025
Merged

Precompute investment objective coefficients#888
tsmbland merged 7 commits into
mainfrom
precompute_coefficients

Conversation

@tsmbland
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland commented Oct 6, 2025

Description

We currently calculate the coefficients (ObjectiveCoefficients, formerly CoefficientsMap) used in the investment algorithm multiple times (every time a new round of appraisal is performed), even though these values won't change.

A better approach is to calculate these upfront for all assets, store them, and pass individual ObjectiveCoefficientss to appraise_investment.

This won't be that impactful currently as the work to calculate the coefficients is small, but this will be more significant once we move the current code for calculating "reduced costs" over to the investment module

Close #885

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 82.92683% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.41%. Comparing base (1cacfaf) to head (44038aa).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/simulation/investment/appraisal.rs 60.00% 4 Missing ⚠️
...rc/simulation/investment/appraisal/coefficients.rs 87.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #888      +/-   ##
==========================================
+ Coverage   85.37%   85.41%   +0.04%     
==========================================
  Files          50       50              
  Lines        5319     5335      +16     
  Branches     5319     5335      +16     
==========================================
+ Hits         4541     4557      +16     
  Misses        559      559              
  Partials      219      219              

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

@tsmbland tsmbland marked this pull request as ready for review October 6, 2025 15:27
Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too!


let mut round = 0;
let objective_type = &agent.objectives[&year];
let mut best_assets: Vec<AssetRef> = Vec::new();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the type hint?

Suggested change
let mut best_assets: Vec<AssetRef> = Vec::new();
let mut best_assets = Vec::new();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the linters say, but sometimes, even though type hints might not be necessary for the program to compile, they are really useful for people reading the code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct that the type annotation isn't required here.

I'm in two minds about this (and giving non-mandatory type annotations in general). On the one hand, not giving them means trimmer code that's easier to change, and if you're using the rust-analyzer plugin in vscode then it gives you these annotations anyway. On the other hand, you are quite reliant on rust-analyzer for readability, which doesn't help when looking at diffs on GitHub

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to merge this as it is, but looking into it I think the idiomatic approach here is to not give the type hint

@tsmbland tsmbland merged commit 8a495f3 into main Oct 9, 2025
8 checks passed
@tsmbland tsmbland deleted the precompute_coefficients branch October 9, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor investment code to calculate CoefficientsMap upfront

3 participants