Add count sales prev n years func

ccao-data / model-res-avm

Automated valuation model for all class 200 residential properties in Cook County (except vacant land and condos)

GNU Affero General Public License v3.0

26 stars 5 forks source link

Add count sales prev n years func #177

Closed wrridgeway closed 8 months ago

wrridgeway commented 8 months ago

This seemed simple enough, but mutate + map solutions were prohibitively slow and multicard sales make joins more verbose than originally anticipated. Considerations are outlined below. Assessment and training sets also need to be handled differently.

wrridgeway commented 8 months ago

Considerations:

should this function be defined within section 3 of the ingest script rather than as a helper?
should the years parameter be added to params.yaml?
i tried rolling the mutate stages that create within_three_years into the summarize functions and for some reason the function slowed down quite a lot

dfsnow commented 8 months ago

should this function be defined within section 3 of the ingest script rather than as a helper?

Yes, moved!

should the years parameter be added to params.yaml?

Yes, added!

i tried rolling the mutate stages that create within_three_years into the summarize functions and for some reason the function slowed down quite a lot

Think it's fine as-is. It's plenty fast, if a little confusing.

@wrridgeway I'm going to merge this for the sake of expediency/testing. Please give this another look/review on Monday.

wrridgeway commented 8 months ago

@dfsnow this all looks good to me. I wish I could have written this out in a cleaner way, but everything else wasn't just a little slower, it was too slow to even use.