Add unit test values for COD, PRD, and PRB based on known good IAAO values #26
Conversation
…d-on-known-good-iaao-values
dfsnow
changed the title
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| AssessPy is a software package for Python developed by the Cook County Assessor’s (CCAO) | |||
| Data Department. It contains many of the functions necessary to perform a standard | |||
| [sales ratio study](https://www.iaao.org/media/standards/Standard_on_Ratio_Studies.pdf). | |||
| [sales ratio study](https://www.iaao.org/wp-content/uploads/Standard_on_Ratio_Studies.pdf). | |||
There was a problem hiding this comment.
Per @jeancochrane, this link is dead, so I updated to the new one.
There was a problem hiding this comment.
These tables and the subsequent test values are pulled directly from the Standard on Ratio Studies.
| @@ -38,7 +68,7 @@ def test_metric_succeeds_on_good_input(self, metric, good_input): | |||
|
|
|||
| def test_metric_met_function_thresholds(self, metric, metric_val): | |||
| if metric == "ki": | |||
| pt.skip("Skipping test for 'ki' metric (ki_met does not exist)") | |||
| return None | |||
There was a problem hiding this comment.
We're not actually skipping a test, we just don't even need to run it. Switching to this has the advantage of not showing a "N skipped tests" warning in the pytest results.
| Assessor’s (CCAO) Data Department. It contains many of the functions necessary | ||
| to perform a standard | ||
| `sales ratio study <https://www.iaao.org/wp-content/uploads/Standard_on_Ratio_Studies.pdf>`_ | ||
|
|
||
| For assessors, we believe that this package will reduce the complexity of | ||
| calculating ratio statistics and detecting sales chasing. We also hope that | ||
| reporters, taxpayers, and members of academia will find this package helpful | ||
| in monitoring the performance of local assessors and conducting research. |
There was a problem hiding this comment.
This text was out-of-date compared to the README, so just quickly copied it over.
assesspy/tests/conftest.py
Outdated
| @pt.fixture(scope="module", params=["1_1", "1_4", "d_1", "d_2"]) | ||
| def iaao_data(request) -> tuple: | ||
| sample = pd.read_csv(FIXTURE_DIR / f"iaao_table_{request.param}.csv") | ||
| return request.param, sample.estimate, sample.sale_price |
There was a problem hiding this comment.
It's really annoying that you have to return the request.param in order to use it in downstream conditionals (here in test_metric_value_is_correct_iaao). However, I couldn't find a good way to get the name of the iaao_data fixture from within test_metric_value_is_correct_iaao. @jeancochrane any thoughts here?
There was a problem hiding this comment.
I think this is fine, to be honest! If you really don't like it you could try following the updated advice in this SO answer and split it out into two fixtures so you can reference the tablename as its own fixture, but I don't feel particularly strongly about which solution is clearer:
@pt.fixture(scope="module", params=["1_1", "1_4", "d_1", "d_2"]):
def iaao_table_name(request):
return request.param
@pt.fixture(scope="module")
def iaao_data(iaao_table_name) -> tuple:
sample = pd.read_csv(FIXTURE_DIR / f"iaao_table_{iaao_table_name}.csv")
return sample.estimate, sample.sale_price
...
def test_metric_value_is_correct_iaao(self, metric, iaao_data, iaao_table_name):
...There was a problem hiding this comment.
Good thinking, I like this a little more. I refactor to use this method in 0d84a32.
assesspy/tests/conftest.py
Outdated
| @pt.fixture(scope="module", params=["1_1", "1_4", "d_1", "d_2"]) | ||
| def iaao_data(request) -> tuple: | ||
| sample = pd.read_csv(FIXTURE_DIR / f"iaao_table_{request.param}.csv") | ||
| return request.param, sample.estimate, sample.sale_price |
There was a problem hiding this comment.
I think this is fine, to be honest! If you really don't like it you could try following the updated advice in this SO answer and split it out into two fixtures so you can reference the tablename as its own fixture, but I don't feel particularly strongly about which solution is clearer:
@pt.fixture(scope="module", params=["1_1", "1_4", "d_1", "d_2"]):
def iaao_table_name(request):
return request.param
@pt.fixture(scope="module")
def iaao_data(iaao_table_name) -> tuple:
sample = pd.read_csv(FIXTURE_DIR / f"iaao_table_{iaao_table_name}.csv")
return sample.estimate, sample.sale_price
...
def test_metric_value_is_correct_iaao(self, metric, iaao_data, iaao_table_name):
...| expected = { | ||
| "1_1": { | ||
| "cod": 29.8, | ||
| "prd": 0.98, | ||
| "prb": 0.232, | ||
| }, | ||
| "1_4": { | ||
| "cod": 14.5, | ||
| "prd": 0.98, | ||
| "prb": 0.135, | ||
| }, | ||
| "d_1": { | ||
| "cod": 7.5, | ||
| "prd": 1.027, | ||
| "prb": -0.120, | ||
| }, | ||
| "d_2": { | ||
| "cod": 7.8, | ||
| "prd": 1.056, | ||
| "prb": -0.011, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[Thought, non-blocking] If we did decide to factor out an iaao_table_name fixture, it might make sense to move these definitions into that fixture so that the table names and their expected values are defined in one place. But again, I think this is probably fine, especially if we don't anticipate needing to reference these values in other tests.
dfsnow
deleted the
25-add-unit-test-values-for-all-metrics-based-on-known-good-iaao-values
branch
This PR adds unit tests for COD, PRD, and PRB using the known good values specified in the IAAO Standard on Ratio Studies example tables.
Closes #25.