Refactor AssessPy for consistency, stability #24

dfsnow · 2024-11-23T02:11:26Z

This PR is a full refactor of the assesspy package designed to make it clearer, more consistent, easier to maintain, and more compatible with other technologies. It was motivated by the work in ccao-data/data-architecture#521, which revealed that the current version of AssessPy is incompatible with Spark.

Warning

This is a breaking refactor. It significantly changes the API of some functions and deprecates others.

Breaking changes

All metrics (COD, PRD, etc.) now have the same inputs (estimate, sale_price) and return the same output (a single float). Previously, some metrics had one input (COD) or different outputs (PRD)
The sub-functions of detect_chasing and is_outlier are no longer exported to the user. Instead they can be selected via an argument in their respective functions
detect_chasing is renamed to is_sales_chased for consistency with is_outlier
Sample datasets are renamed to reflect their respective sources

Other changes

Removed as much numpy as possible for compatibility with Spark/Athena
Added static types to almost everything, which should make this package easier to maintain in the long run
Replaced all unit tests with a fixtures matrix and parameters
Updated a lot of the documentation structure
Updated the example ratio study notebook

Considering the size of this PR, I recommend ignoring most of the removed code and considering only new stuff. Treat it like a greenfield package.

Closes #15.

dfsnow · 2024-11-23T05:28:09Z

assesspy/ci.py

+        Default 0.05. Float value indicating the confidence
        interval to return. 0.05 will return the 95% confidence interval.


I updated lot of the language just to reflect Python (instead of R) types.

dfsnow · 2024-11-23T05:36:04Z

assesspy/data/ccao_sample.parquet

All the data files contain exactly the same content, they're just renamed and reorganized (I removed unneeded columns).

dfsnow · 2024-11-23T05:36:35Z

assesspy/tests/conftest.py

I basically rewrote all the tests and only kept the known, good values for each metric to test against. Using pytest fixtures lets us simplify the test code a lot while running more tests overall.

dfsnow · 2024-11-23T05:39:00Z

assesspy/__init__.py

+from .outliers import is_outlier
+from .sales_chasing import is_sales_chased


I decided to hide of some of the exported functions here (iqr_outlier, quantile_outlier) to keep things in line with how is_sales_chased works. Both function are accessible via is_outlier anyways.

dfsnow · 2024-11-23T05:41:09Z

assesspy/ci.py

-def prd_ci(assessed, sale_price, nboot=100, alpha=0.05):
-    return boot_ci(
-        prd, assessed=assessed, sale_price=sale_price, nboot=nboot, alpha=alpha
+def prb_ci(


The PRB CI was accessible before, but only via a dictionary value returned by prb(). This wasn't consistent with the other functions and also meant there was no prb_ci() function. I've added one here for API consistency even though it's largely redundant code with PRB.

[Suggestion, non-blocking] If we wanted to reduce duplication between this function and metrics.prb(), we could factor out a shared helper function that validates the inputs and returns the OLS model object for each function to interpret as needed. Not a high priority in my opinion though, since we're only duplicating logic in two places.

dfsnow · 2024-11-23T05:42:10Z

assesspy/load_data.py

+from importlib.resources import as_file, files
+


This is apparently the hot new way to import package data. setuptools was complaining to me about pkg_resources being deprecated.

dfsnow · 2024-11-23T05:42:43Z

assesspy/load_data.py

+        return pd.read_parquet(file)
+
+
+def quintos_sample() -> pd.DataFrame:


I figured it would be nice to include the Quintos data as an export to use in tests and demos.

dfsnow · 2024-11-23T05:43:24Z

assesspy/metrics.py

+def cod(
+    estimate: Union[list[int], list[float], pd.Series],
+    sale_price: Union[list[int], list[float], pd.Series],
+) -> float:


cod() now gets 2 input args instead of the single ratio arg it took before. FINALLY.

dfsnow · 2024-11-23T05:45:53Z

assesspy/metrics.py

+    estimate = (
+        pd.Series(estimate, dtype=float)
+        .rename("estimate")
+        .reset_index(drop=True)
+    )
+    sale_price = (
+        pd.Series(sale_price, dtype=float)
+        .rename("sale_price")
+        .reset_index(drop=True)
+    )
+    df = pd.concat([estimate, sale_price], axis=1)
+    # Mergesort is required for stable sort results
+    df.sort_values(by="sale_price", kind="mergesort", inplace=True)
+    df.reset_index(drop=True, inplace=True)


The index manipulation here is annoyingly important. If you pass two Series as inputs to this function and they have different indexes, the result will be a NaN output (due to index joining) unless you reset the indices.

dfsnow · 2024-11-23T05:46:57Z

assesspy/metrics.py

+
+
+def cod(
+    estimate: Union[list[int], list[float], pd.Series],


Using Union here instead of the newer list[int] | list[float] format for compatibility with Python 3.9.

dfsnow · 2024-11-23T05:48:33Z

assesspy/outliers.py


-import numpy as np
-from scipy import stats


I don't know why we needed a scipy dependency to calculate the IQR when we can (presumably) do the same thing manually with Panda's quantile(). If there's a good reason we need this specific IQR function then we can add it back.

dfsnow · 2024-11-23T05:52:25Z

assesspy/sales_chasing.py

    pct_actual = pct_in_range(ratio, bounds[0], bounds[1])

-    return pct_actual > pct_ideal
+    return bool(abs(pct_actual - pct_ideal) > gap)


I added a new requirement that the absolute difference in percentages be greater than some threshold because this method was waaay too sensitive before.

dfsnow · 2024-11-23T06:06:11Z

assesspy/tests/test_metrics.py

+    def test_metric_value_is_correct(self, metric, metric_val):
+        expected = {
+            "cod": 17.81456901196891,
+            "prd": 1.0484192615223522,
+            "prb": 0.0009470721642262903,
+            "mki": 0.794,
+            "ki": -0.06,
+        }
+        assert pt.approx(metric_val, rel=0.02) == expected[metric]


IMO it's worth updating these tests to use known good values rather than just values resulting from our sample data. I opened #25 for this purpose.

jeancochrane

Awesome work here 👏🏻 Code is indeed way cleaner, I can see why the rewrite was worth it!

CITATION.cff

assesspy/ci.py

assesspy/metrics.py

jeancochrane · 2024-11-25T16:49:00Z

assesspy/metrics.py

+    return gini_assessed, gini_sale_price
+
+
+def mki(


[Thought, non-blocking] Unrelated to this PR, but these changes are reminding me that we should make a note to add MKI/KI/Gini to our sales ratio glossary considering we're using them more and more.

assesspy/metrics.py

assesspy/tests/conftest.py

jeancochrane · 2024-11-25T17:19:26Z

assesspy/utils.py

-            raise Exception("All input vectors must be numeric.")
-        if check.isnull().values.any():
-            out.append("\nInput vectors contain null values.")
+            out_msg.append("\nAll input values must be numeric.")


[Nitpick, non-blocking] Instead of prepending each error string with a newline, could we just add newlines as separators during the join()?

Holdover from the old code. Fixed in 085dcac.

jeancochrane · 2024-11-25T17:20:17Z

assesspy/utils.py

-        raise Exception("".join(map(str, out)))
+    out_msg_set = set(out_msg)
+    if len(out_msg_set) > 1:
+        raise Exception("".join(map(str, out_msg_set)))


[Question, non-blocking] What motivates the str() cast here? it kind of seems like all of the elements of out_msg_set are strings already, right?

Likewise a holdover from the old code. Removed!

dfsnow added 30 commits November 20, 2024 17:24

Add type stubs to dev deps

e059e35

Refactor all assesspy formulas

27ac5e7

Add full test suite for metrics

68faba6

Add and clean testing data/samples

51d68e4

Update doc refs for data sets

8515e9a

Update metric docs types

c04fe4b

Cleanup doc groupings

dc8dc25

Bump package version

0d48492

Add metrics _met docs

8f32aa9

Update all CI functions

926199f

Don't export check_inputs

e4d5431

Update outlier functions

0aa5310

Cleanup test fixtures

fcec1a2

Refactor outlier functions

072c580

Rename expect output vars

d222422

Add outlier function tests

ebbd574

Add option to disable lt 0 check for check_inputs

f76b773

Update outliers docs heading

a566af8

Remove scipy dependency

0b8e661

Refactor sales chasing functions

db811c8

Fixup outlier functions

67f82bf

Finalize sales chasing rewrite

399f49c

Add tests for all code paths

0ea46a1

Update README

0b5a17c

Update docs

9839114

Lint with ruff

7603e84

Add Python 3.13 support

1926f7c

Update tests and types

dee2abe

Add Python 3.8 support

a818cde

Add more python versions, update docstrings

076ce1e

dfsnow commented Nov 23, 2024

View reviewed changes

Remove 3.8 from tox env list

07dcf8c

dfsnow commented Nov 23, 2024

View reviewed changes

dfsnow requested review from wrridgeway and jeancochrane November 23, 2024 05:54

dfsnow self-assigned this Nov 23, 2024

dfsnow marked this pull request as ready for review November 23, 2024 05:55

dfsnow mentioned this pull request Nov 23, 2024

Add unit test values for all metrics based on known good IAAO values #25

Closed

dfsnow commented Nov 23, 2024

View reviewed changes

jeancochrane approved these changes Nov 25, 2024

View reviewed changes

dfsnow added 7 commits November 25, 2024 19:14

Bump release date

2da4d60

Remove unnecessary typing

1f5ea2b

Deduplicate PRD code

a970fe3

Scope test to session

6715a50

Join check_inputs messages with newlines

085dcac

Fix ruff errors

0dec842

Scope seed fixture to each test

0c21827

dfsnow merged commit d611de4 into main Nov 25, 2024
14 checks passed

dfsnow deleted the dfsnow/full-refactor branch November 25, 2024 21:16

dfsnow mentioned this pull request Nov 27, 2024

Fix PRB intercept and CI sampling #27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor AssessPy for consistency, stability #24

Refactor AssessPy for consistency, stability #24

dfsnow commented Nov 23, 2024 •

edited

Loading

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024 •

edited

Loading

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

jeancochrane left a comment

jeancochrane Nov 25, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 25, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 25, 2024

		Default 0.05. Float value indicating the confidence
		interval to return. 0.05 will return the 95% confidence interval.

		from .outliers import is_outlier
		from .sales_chasing import is_sales_chased

		return pd.read_parquet(file)


		def quintos_sample() -> pd.DataFrame:

Refactor AssessPy for consistency, stability #24

Refactor AssessPy for consistency, stability #24

Conversation

dfsnow commented Nov 23, 2024 • edited Loading

Breaking changes

Other changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow commented Nov 23, 2024 •

edited

Loading

dfsnow Nov 23, 2024 •

edited

Loading