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

fix(rust, python): Fix match on last item for join_asof with strategy="nearest" #11673

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

mcrumiller
Copy link
Contributor

Resolves #11649

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Oct 11, 2023
@mcrumiller mcrumiller requested a review from orlp as a code owner October 11, 2023 21:48
@@ -259,10 +258,15 @@ pub(super) fn join_asof_nearest_with_tolerance<
break;
}
} else {
// We'ved moved farther away, so the last element was the match.
out.push(Some(offset - 1));
// We'ved moved farther away, so the last element was the match if it's within tolerance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code previously was simply accepting that the prior match was a success without making sure it was eligible, ditto for the groups version below.

@orlp
Copy link
Collaborator

orlp commented Oct 11, 2023

@mcrumiller I had assigned that issue to me :( I'm working a refactor for this that also cleans everything up.

@mcrumiller
Copy link
Contributor Author

mcrumiller commented Oct 11, 2023

@mcrumiller I had assigned that issue to me :( I'm working a refactor for this that also cleans everything up.

Ahh no problem. I wrote the original code so I felt responsible (I am) for the error. Should I just close this?

@orlp
Copy link
Collaborator

orlp commented Oct 12, 2023

I suppose we can merge it to ensure the bug is fixed sooner and it adds tests. I'll deal with the rebase errors.

@orlp orlp enabled auto-merge (squash) October 12, 2023 07:24
@orlp orlp merged commit da08773 into pola-rs:main Oct 12, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

join_asof tolerance parameter gives wrong results
3 participants