Use worst case method for MKI / KI

[Damonamajor]

This uses a worst case scenario method for identifying MKI / KI values in order to ensure reproducibility.

[Damonamajor]

Uses False for ascending order for estimate in accordance with our external guidance.

After a lot of deliberation, we decided the best way forward is to assume the "worst case scenario" in terms of MKI/KI metrics by sorting the data first by the ascending actual value (sale price) and then by the descending predicted value (modeled result). Not saying this has to be your solution, but wanted to share our thinking if helpful.

[jeancochrane]

Great work here! Overall I think this looks right, just a few small nitpicks below related to the test definition.

[jeancochrane]

[Nitpick, optional] For consistency with other tests, I think it would make sense to switch to using a fixture here. Note how the unchanged tests above include the quintos_data fixture via a function parameter -- this is technically a fixture definition that inherits from the quintos_data fixture, but the principle is the same as if we were including the fixture in a test:

assesspy/assesspy/tests/test_metrics.py

Lines 11 to 15 in a0d0359

	@pt.fixture
	def metric_val(self, metric, ccao_data, quintos_data):
	if metric in ["mki", "ki"]:
	return getattr(ap, metric)(*quintos_data)
	return getattr(ap, metric)(*ccao_data)

Here's the definition of the quintos_data fixture, which pytest loads automatically from conftest.py on startup so it can pass the fixture into any fixture or function that includes it in its function parameters:

assesspy/assesspy/tests/conftest.py

Lines 24 to 27 in a0d0359

	@pt.fixture(scope="session")
	def quintos_data() -> tuple:
	sample = ap.quintos_sample()
	return sample.estimate, sample.sale_price

If we follow my recommendation above to save the new data in a new sample file, we would need to define a new quintos_data_with_tiebreaks fixture in conftest.py and then include it here:

Suggested change

	@pt.mark.parametrize("metric", ["mki", "ki"])
	def test_quintos_metric_matches_across_estimates(metric):
	"""
	For the quintos dataset, MKI/KI should be identical based
	on the ordering of estimates.
	"""
	sample = ap.quintos_sample()
	@pt.mark.parametrize("metric", ["mki", "ki"])
	def test_quintos_metric_matches_across_estimates(metric, quintos_data_with_tiebreaks):
	"""
	For the quintos dataset, MKI/KI should be identical based
	on the ordering of estimates.
	"""
	sample = quintos_data_with_tiebreaks

The change should be pretty similar if we stick with quintos_data -- we just wouldn't need the extra fixture definition in confest.py in that case, since the quintos_data fixture already exists.

[Damonamajor]

We can index, but this feels easier to interpret later.

[jeancochrane]

The new version of the test/fixture looks great! Some suggestions below, mostly just tweaks to documentation to make the purpose of this change clearer.

[jeancochrane]

[Question, optional] I wonder if this kwarg is still necessary? Per the pandas docs, kind is only used when sorting on a single column, but now we're sorting on two columns:

Choice of sorting algorithm. See also numpy.sort() for more information. mergesort and stable are the only stable algorithms. For DataFrames, this option is only applied when sorting on a single column or label.

I'm agnostic as to whether we should leave the kwarg in or take it out -- it doesn't seem to make anything worse to leave it in, and it could provide a layer of defensiveness preventing us from accidentally reintroducing an unstable sort if we ever decide to switch back to sorting on a single column -- but I'd be interested to see if the tests still pass when we take it out.

[Damonamajor]

I assumed that it wouldn't affect anything. The only reason I left it in was if we ever wanted to do something with the dataset externally, it would remain the same. Maybe we wanted to look at class once sorted by MKI. That's not really a good example, but I could imagine something along these lines.

I expect it to pass even without it.

[jeancochrane]

I'm fine leaving it in!