EnergySystemsModellingLab / EnergySystemsModellingLab/MUSE2

`compare_approx` breaks transitivity

Open
#1,413 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.