Skip to content

Conversation

@Aurashk
Copy link
Collaborator

@Aurashk Aurashk commented Nov 7, 2025

Description

Add a small epsilon value to all activity coefficients in the NPV appraisal optimisation to help prevent spurious investment failures.

Fixes #991

see #998 for a results comparison with simple model

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
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.12%. Comparing base (c64a7a8) to head (26d18c2).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...rc/simulation/investment/appraisal/coefficients.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
+ Coverage   83.94%   84.12%   +0.17%     
==========================================
  Files          52       52              
  Lines        5794     5844      +50     
  Branches     5794     5844      +50     
==========================================
+ Hits         4864     4916      +52     
+ Misses        695      689       -6     
- Partials      235      239       +4     

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

@Aurashk Aurashk changed the title add temporary npv enabled model for testing add epsilon term to npv activity coefficients Nov 11, 2025
@Aurashk Aurashk marked this pull request as ready for review November 11, 2025 10:08
@Aurashk Aurashk requested review from dalonsoa and tsmbland November 11, 2025 10:39
Copy link
Collaborator

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

Looks good!

let capacity_coefficient = -annual_fixed_cost(asset);

// Small epsilon to ensure break-even assets are dispatched
let epsilon = MoneyPerActivity(1e-14);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would define this as a const at the top of the function. E.g. see

const MAX_DISPLAY: usize = 10;

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also use f64::EPSILON, so it is not an arbitrary small number. See https://doc.rust-lang.org/std/f64/constant.EPSILON.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's interesting didn't know about that. I changed it to f64::EPSILON * 100.0 because we need it to big enough to force out precision errors. Multiplying by 100.0 is still a bit arbitrary but seems more elegant this way

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.

Add small offset to activity coefficients NPV

4 participants