improve treatment of equal-metric appraisals

[Aurashk]

• Right now there is a hardcoded approach to prioritising appraisals with equally-good metrics in AppraisalOutput::compare_with_equal_metrics. We may want to make this configurable

• If we still can't single out a single-best appraisal we always pick the first element in the container in select_from_assets_with_equal_metric. We may also want to make this configurable

• It might make more sense to remove the call to AppraisalOutput::compare_with_equal_metrics in AppraisalOutput::compare_metric and independently sort by the equal-metric criteria in select_from_assets_with_equal_metric instead of relying on the initial sort with compare_metric.

• Since the rules are getting more complex might be nice to have add testing for this functionality with an array of appraisals as a fixture which checks the asset we expect is chosen

[alexdewar]

I'm not totally clear on what still needs to be done for this, but I think we should figure it out, so I've put it in the latest iteration

[Aurashk]

@alexdewar
To elaborate on this further now I've had a closer look:
The top two points about configuring equal-metric behaviour originate from this comment. #1006 (comment)
In the equal-metrics case, we've assumed that agents always prefer newer assets over older, and commissioned over non-commissioned. It may be that users want to swap these rules around/ turn them off. I also find a random option mentioned quite interesting, it could used for otherwise-identical model runs to see if equal-metric selections impact the results in a significant way. This is speculative though and might not be useful for anyone, they can be crossed off if you think that's the case. Otherwise we can put it in a separate issue

-Third point can be crossed since the function in question is removed, the idea was to separate equal-metric logic from non-equal-metric logic better, but it's no longer a concern from the latest refactoring

-Fourth point might still be nice, the idea would be to create different lists of ApprasialOutput for each test case using some helper function. Then checking the asset we expect is selected (and we correctly deal with the corner cases such as NaN metrics). Particularly if we do decide to make the asset selection behaviour configurable as above this would be important.
@tsmbland just tagging if you wanted to add any thoughts

[alexdewar]

@alexdewar To elaborate on this further now I've had a closer look: The top two points about configuring equal-metric behaviour originate from this comment. #1006 (comment) In the equal-metrics case, we've assumed that agents always prefer newer assets over older, and commissioned over non-commissioned. It may be that users want to swap these rules around/ turn them off. I also find a random option mentioned quite interesting, it could used for otherwise-identical model runs to see if equal-metric selections impact the results in a significant way. This is speculative though and might not be useful for anyone, they can be crossed off if you think that's the case. Otherwise we can put it in a separate issue

-Third point can be crossed since the function in question is removed, the idea was to separate equal-metric logic from non-equal-metric logic better, but it's no longer a concern from the latest refactoring

-Fourth point might still be nice, the idea would be to create different lists of ApprasialOutput for each test case using some helper function. Then checking the asset we expect is selected (and we correctly deal with the corner cases such as NaN metrics). Particularly if we do decide to make the asset selection behaviour configurable as above this would be important. @tsmbland just tagging if you wanted to add any thoughts

Sorry, I missed this before. Tbh I think making this configurable is just making work for ourselves. Users probably won't care what we do, provided we do something non-stupid and non-random, so we should just pick one strategy and go with that. Otherwise we're just making our lives harder unnecessarily. If users don't like it -- and they have a good reason -- we can just change to a different strategy. More configuration options means more code to test and maintain, so it's better to avoid it unless there's a good reason to do otherwise IMO.

In terms of the random option: I think a better way of achieving the same thing would be to do a sensitivity analysis (i.e. checking what happens when you vary parameters).

[Aurashk]

Thanks @alexdewar, the only other thing is adding some tests for AppraisalOutputs then. If we want them - I can open a PR for that which will close this issue?

[alexdewar]

@Aurashk That would be great, if you're volunteering 😄. It's just the metric and asset fields of AppraisalOutput that are relevant so the maps etc. can just be empty. And remember that we have fixtures for assets in fixture.rs that you may be able to reuse.