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

fixed autocorr + Lempel Ziv Complexity + Some failing tests #67

Merged
merged 6 commits into from
Oct 9, 2023

Conversation

abstractqqq
Copy link
Collaborator

@abstractqqq abstractqqq commented Oct 8, 2023

The issue is that in lazy Polars, slice behaves differently. The problem lies somewhere in its length calculation. Try running the following code, which will give us error in the lazy case. Since in our original code, we are just returning one expression at the end, the error didn't occur, but something inside was wrong. You can see the issue here: pola-rs/polars#11594 (comment)

df = pl.DataFrame({
    "a": [1,2,1,2,1,2],
    "b": [None, None, 1,1,1,1]
})

df.select(
    pl.col("a").slice(offset = 0, length=pl.count() - 2).alias("1"),
    pl.col("a").slice(offset = 1, length=pl.count() - 2).alias("2")
)

df.lazy().select(
    pl.col("a").slice(offset = 0, length=pl.count() - 2).alias("1"),
    pl.col("a").slice(offset = 1, length=pl.count() - 2).alias("2")
).collect()

I fixed the bug by reverting back to using shift, which will be slower than .head() / tail() / slice(), but will avoid using slice for now. We actually do not need to drop nulls after shifting, because dot product will null will just result in 0.

@vercel
Copy link

vercel bot commented Oct 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
functime-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 2:35am

@abstractqqq abstractqqq changed the title fixed autocorr fixed autocorr + Lempel Ziv Complexity + Some failing tests Oct 8, 2023
Copy link
Contributor

@topher-lo topher-lo left a comment

Choose a reason for hiding this comment

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

Just renamed value to threshold

changed to threshold

Co-authored-by: Chris Lo <[email protected]>
@abstractqqq
Copy link
Collaborator Author

@MathieuCayssol Autocorrelation should be correct now. Please take a look

@MathieuCayssol
Copy link
Collaborator

Sure, I will have a look. For the unit tests, the things you've updated, I already did it in the branch #66

@MathieuCayssol MathieuCayssol merged commit c5037ba into main Oct 9, 2023
@MathieuCayssol MathieuCayssol deleted the fix/issue_65 branch October 9, 2023 20:46
topher-lo pushed a commit that referenced this pull request Dec 19, 2023
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