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(#480): pure NaNs when flattening with lookbehind-tuple in 01_basic #489

Conversation

HLasse
Copy link
Collaborator

@HLasse HLasse commented Feb 22, 2024

fix(#480): pure NaNs when flattening with lookbehind-tuple in 01_basic

Fixes #480

fix: bug in _get_timedelta_frame if timestamp col names were the same

Copy link
Collaborator Author

HLasse commented Feb 22, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@HLasse HLasse enabled auto-merge February 22, 2024 14:50
@HLasse HLasse merged commit 6ffbd92 into main Feb 22, 2024
12 checks passed
@HLasse HLasse deleted the fix/480/pure_NaNs_when_flattening_with_lookbehind-tuple_in_01_basic branch February 22, 2024 14:50
Copy link

codspeed-hq bot commented Feb 22, 2024

CodSpeed Performance Report

Merging #489 will not alter performance

Comparing fix/480/pure_NaNs_when_flattening_with_lookbehind-tuple_in_01_basic (3ade969) with main (70da39a)

Summary

✅ 7 untouched benchmarks

Copy link
Collaborator

@MartinBernstorff MartinBernstorff left a comment

Choose a reason for hiding this comment

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

Nasty bug, nice to catch it now! Unsure why this wasn't caught by other tests?

joined_frame = predictiontime_frame.df.join(
value_frame.df, on=predictiontime_frame.entity_id_col_name, how="left"
# ensure that the timestamp col names are different to avoid conflicts
unique_predictiontime_frame_timestamp_col_name = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the terminology here is a bit confusing; it isn't unique, but rather it is different from the one in the value_frame, right?

If that's the case, we might want to guard it to clarify intent. E.g. if predictiontime_frame.timestamp_col_name == value_frame.timestamp_col_name

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.

fix: pure NaNs when flattening with lookbehind-tuple in 01_basic
2 participants