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 DateSplitter for multiples of base frequencies #2500

Merged
merged 1 commit into from
Dec 17, 2022

Conversation

lostella
Copy link
Contributor

@lostella lostella commented Dec 16, 2022

Issue #, if available: Fixes #2499

Description of changes:

  • Simplify the logic of date splitting, by using the newly introduced periods_between
  • Render to_integer_slice pointless (can be removed in a separate breaking change)
  • Also render TimeSeriesSlice pointless (slice_data_entry is sufficient)
  • Extend and consolidate tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup

@lostella lostella added bug fix (one of pr required labels) pending v0.11.x backport This contains a fix to be backported to the v0.11.x branch labels Dec 16, 2022
src/gluonts/dataset/split.py Outdated Show resolved Hide resolved
test/dataset/test_split.py Outdated Show resolved Hide resolved
@lostella lostella merged commit e152659 into awslabs:dev Dec 17, 2022
@lostella lostella deleted the fix-2499 branch December 17, 2022 08:24

The frequency is taken from ``start``.
"""
return ((end - start).n // to_offset(start.freq).n) + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to call to_offset here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think this can be removed

end: pd.Period,
) -> int:
"""
Counts how many periods fit between ``start`` and ``end``
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "count ...".

I think an example would be great

lostella added a commit to lostella/gluonts that referenced this pull request Dec 20, 2022
@lostella lostella mentioned this pull request Dec 20, 2022
lostella added a commit that referenced this pull request Dec 20, 2022
* Fix dataclass handling of member inheritance. (#2492)

* Fix: sort dataset keys in error message when importing non-existing dataset (#2497)

Co-authored-by: Lorenzo Stella <[email protected]>
Co-authored-by: Jasper <[email protected]>

* Fix `DateSplitter` for multiples of base frequencies (#2500)

* Fix docstrings according to docformatter (#2501)

* Pin docformatter version. (#2507)

* Docs: Fix install instructions. (#2508)

* Add examples to docstring for `periods_between` (#2504)

* Cap numpy compatibility in `mxnet` extra requirements (#2506)

* xfail multivariate grouper test

Co-authored-by: Lorenzo Stella <[email protected]>
Co-authored-by: Jasper <[email protected]>

* remove undesired files

* `itertools.select`. (#2426)

* Add itertools select.

Co-authored-by: Jasper <[email protected]>
Co-authored-by: Lorenzo Stella <[email protected]>
Co-authored-by: Jasper <[email protected]>
@lostella lostella removed the pending v0.11.x backport This contains a fix to be backported to the v0.11.x branch label Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix (one of pr required labels)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect test split start point and length when using frequency with unit >1
2 participants