Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diff order fix #121

Merged
merged 2 commits into from
Nov 23, 2022
Merged

Diff order fix #121

merged 2 commits into from
Nov 23, 2022

Conversation

pdurham2
Copy link
Contributor

@pdurham2 pdurham2 commented Oct 29, 2022

Corrected issue where the diff order was hard coded as 2 in lad_filtering. Also added test_lad_filtering_scoring_diff_order to test_models which uses the last data points, takes the appropriate diff, and then compares to the adjusted actual to make sure the appropriate diff order is applied.

Related Issue: #120
@sayanchk for review

Copy link
Collaborator

@sayanchk sayanchk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Nice to have a unit test to test the diff_order.

Great work @pdurham2 ! Thanks for contributing to Luminaire!

@sayanchk
Copy link
Collaborator

sayanchk commented Nov 2, 2022

@shahsmit14 Changes are approved. This is an important bug fix addressing a prediction bias issue under higher order differencing for the filtering model and should improve the model performance. This should go as a part of the next patch.

@sayanchk sayanchk requested a review from shahsmit14 November 2, 2022 07:17
@shahsmit14
Copy link
Collaborator

shahsmit14 commented Nov 4, 2022

@sayanchk If we want this to be released, we need the update for the version in setup.py.
@pdurham2 - Can you also add that in the same PR? new version will be 0.4.2 and the file to update will be https://github.com/zillow/luminaire/blob/master/setup.py#L17

@shahsmit14
Copy link
Collaborator

I'm just going to merge this and create a release MR.

@shahsmit14 shahsmit14 merged commit 72a05d3 into zillow:master Nov 23, 2022
@shahsmit14
Copy link
Collaborator

now available in https://pypi.org/project/luminaire/0.4.2/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants