EnergySystemsModellingLab / EnergySystemsModellingLab/MUSE2
`compare_approx` breaks transitivity
- Dominant language
- Rust
- Stars
- 8
- Forks
- 5
- Avg merge
- 1d 2m
- Merged PRs (30d)
- 37
Description
We use `compare_approx` when comparing appraisal metrics so that assets with _approximately equal_ metrics are considered equal (then falling back to a series of secondary criteria, such as favouring existing assets over new ones). This is to ensure that we don't get changes in investment decisions caused by tiny floating point differences (e.g. on different operating systems).
Unfortunately, there's a flaw here. Consider the following three assets (real example from #1402):
- Asset 27: `26.871084337032230`
- Asset 28: `26.871084337032244`
- Asset 32: `26.871084337032247`
Using `compare_approx`, you get the following:
- 27 vs 28 -> equal
- 28 vs 32 -> equal
- 27 vs 32 -> less than
The transitivity rule is that if `a == b` and `b == c`, then `a == c` must also be true.
If there's an inconsistency and this transitivity rule doesn't hold, then `sort_by` (called [here](https://github.com/EnergySystemsModellingLab/MUSE2/blob/d588b7632d36b568b38c9b2eb583a94aee17b114/src/simulation/investment/appraisal.rs#L323)) will detect this inconsistency and panic.
This is hopefully quite a rare problem, but it's cropping up in #1402, and something that we absolutely need to address. It's also platform dependent - the example for #1402 isn't panicking for me on my mac, as the appraisal metrics are slightly different so transitivity isn't broken, but is panicking for Adam on WSL.
A naive approach might be to use slightly lower precision in `compare_approx`. That doesn't fix the underlying problem, but would hopefully make it rarer. Alternatively we could round to fixed precision before comparing. There's probably a better solution though. This slightly reminds me of some of the issues with #1296, so there may be some inspiration there.
Contributor guide
Research direction
Start with src/simulation/investment/appraisal.rs at the sort_by call around line 323, then trace the compare_approx implementation and review the related discussion in #1296. Investigate a comparison strategy that preserves transitivity for the appraisal metrics and verify that the assets in the reported example sort without a panic across differing floating-point results.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100