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

Does quotes.Aggregate(PeriodSize.Week) use a fixed calendar week? #1184

Open
3 tasks
DaveSkender opened this issue Apr 1, 2024 · 5 comments
Open
3 tasks
Labels
documentation Improvements or additions to documentation enhancement New feature or request question Question and/or answer

Comments

@DaveSkender
Copy link
Owner

DaveSkender commented Apr 1, 2024

The quotes.Aggregate(PeriodSize.Week) utility likely uses a rolling 7-day week instead of a fixed week with a specific day (e.g. Saturday) as the end -- which may be more preferred in most use cases.

  • confirm its behavior with additional unit tests
  • update documentation to clarify exactly how it boxes dates at each size
  • consider providing overloads to allow both rolling or fixed calendar windows (e.g. with CalendarWeekRule)

Originally posted by @DaveSkender in #1183 (comment)

@DaveSkender DaveSkender added documentation Improvements or additions to documentation enhancement New feature or request question Question and/or answer labels Apr 1, 2024
@DaveSkender DaveSkender changed the title Does quotes.Aggregate(PeriodSize.Week) user a fixed calendar week? Does quotes.Aggregate(PeriodSize.Week) use a fixed calendar week? Apr 1, 2024
@DaveSkender
Copy link
Owner Author

DaveSkender commented Apr 2, 2024

The fix here is likely to make the non-TimeSpan variant of this method only use fixed calendar windows, like we do in our regular week block code for Pivot Points.

... switch {
  PeriodSize.Month => d.Month,
  PeriodSize.Week => EnglishCalendar.GetWeekOfYear(d, EnglishCalendarWeekRule, EnglishFirstDayOfWeek),
  ...

to do

  • only use fixed calendar window, not the rolling 7-day period, for week
  • update and clarify in documentation
  • test that sub-week variants can be done this way as well. It may require more def on CalendarWeekRule as the arbitrary starting date e.g. for 3-day / 15-minute. Do we need one rule for each window size?

@hmuhdkamran

This comment was marked as off-topic.

@DaveSkender

This comment was marked as off-topic.

@hmuhdkamran

This comment was marked as off-topic.

@DaveSkender

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request question Question and/or answer
Projects
Development

No branches or pull requests

2 participants