Skip to content

Conversation

@nicolasdalsass
Copy link
Contributor

@nicolasdalsass nicolasdalsass commented Jan 9, 2025

QS and QE were missing, Y in some pandas versions is an alternative to A

Repro project :
DKU_TUT_VISUALIZATION_11.zip

The repro project uses a python recipe to generate data, here are various date arrays useful for testing cases :

['2022-01-01', '2022-07-01', '2023-01-01', '2023-07-01', '2024-01-01', '2024-07-01', '2025-01-01', '2025-07-01', '2026-01-01']
['2022-12-31', '2023-12-31', '2024-12-31', '2025-12-31', '2026-12-31', '2027-12-31', '2028-12-31', '2029-12-31', '2030-12-31']
['2022-07-31', '2022-08-31', '2022-09-30', '2022-10-31', '2022-11-30', '2022-12-31', '2023-01-31', '2023-02-28', '2023-03-31']
['2022-07-01', '2022-08-01', '2022-09-01', '2022-10-01', '2022-11-01', '2022-12-01', '2023-01-01', '2023-02-01', '2023-03-01']
['2022-01-01', '2022-04-01', '2022-07-01', '2022-10-01', '2023-01-01', '2023-04-01', '2023-07-01', '2023-10-01', '2024-01-01']
['2022-07-31', '2022-10-31', '2023-01-31', '2023-04-30', '2023-07-31', '2023-10-31', '2024-01-31', '2024-04-30', '2024-07-31']

@nicolasdalsass nicolasdalsass self-assigned this Jan 9, 2025
@nicolasdalsass nicolasdalsass changed the title Fix conversion to rolling compatible date time for some frequencies (… Fix conversion to rolling compatible date time for some frequencies Jan 9, 2025
Copy link
Contributor

@Marsobad Marsobad left a comment

Choose a reason for hiding this comment

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

Tested and LGTM for quarterly and yearly data!
Thanks for the fix, I'll let you decide if it's worth fixing business days as well

test state comment
quarterly data  
semi annual  
yearly data  
business days let’s confirm we can extrapolate 1 business day = 7/5 calendar day

elif time_unit == 'quarters' or time_unit.startswith("Q"):
return 91 * time_step, 'days'
# There is no half year frequency, those are inferred as two quarters instead in infer_frequency()
elif time_unit == 'years' or time_unit.startswith("Y") or time_unit.startswith("A"):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion : we could take this opportunity to also support business days
Screenshot 2025-01-16 at 18 32 08
I guess we can assume 1 business day = 7/5 days?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did you manage to business days as your infered frequency ? :-D putting monday to friday repeatedly in the dataset ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the proposition otherwise, i'm just surprised it occured 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having a go at this change, and in fact I don't think we should proceed with 7/5 days - the way the plugin works, if you window for example over 4 days with a business days dataset, we will in the end window over ceil(4 * 5/7) = 3 days.

Considering the user may not even realize we inferred business days in the first place, this seems very unintuitive, and I think I'd rather raise a clean error, WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I agree!

@Marsobad Marsobad self-requested a review January 24, 2025 14:15
Copy link
Contributor

@Marsobad Marsobad left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolasdalsass nicolasdalsass merged commit 144438f into master Jan 24, 2025
@nicolasdalsass nicolasdalsass deleted the bug/some_frequencies_dont_window_correctly branch January 24, 2025 14:43
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