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.

Sign up for GitHub

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

Jump to bottom

time_granularity -> freq #172

Merged
merged 2 commits into from
Jul 4, 2019
Merged

time_granularity -> freq #172

merged 2 commits into from
Jul 4, 2019

Conversation

lostella
Copy link
Contributor

@lostella lostella commented Jul 4, 2019

Issue #, if available: #134

Description of changes: s/time_granularity/freq/g

NOTE: this is a breaking change.

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

@lostella lostella added the API change This item involves API change label Jul 4, 2019
geoalgo
geoalgo previously approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@geoalgo geoalgo left a comment

Choose a reason for hiding this comment

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

Very cool! Thanks for doing this :)
Can you just check that 'time_granularity' is gone from every folder?

@lostella
Copy link
Contributor Author

lostella commented Jul 4, 2019

Can you just check that 'time_granularity' is gone from every folder?

I did. However, it looks like the host producing the docs is now failing, because in the notebook there is "regenerate=False", so probably the datasets are kept on the machine and the metadata file still has time_granularity...

@szha
Copy link
Member

szha commented Jul 4, 2019

Job PR-172/2 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-172/2/index.html

@codecov-io
Copy link

codecov-io commented Jul 4, 2019 •
edited

Codecov Report

Merging #172 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #172   +/-   ##
=======================================
  Coverage   78.53%   78.53%           
=======================================
  Files         124      124           
  Lines        6783     6783           
=======================================
  Hits         5327     5327           
  Misses       1456     1456
Impacted Files Coverage Δ
src/gluonts/dataset/repository/_util.py 33.33% <ø> (ø) ⬆️
...c/gluonts/dataset/artificial/generate_synthetic.py 12.94% <0%> (ø) ⬆️
src/gluonts/dataset/common.py 81.02% <33.33%> (ø) ⬆️
src/gluonts/dataset/artificial/_base.py 31.8% <66.66%> (ø) ⬆️

aalexandrov
aalexandrov suggested changes Jul 4, 2019
View reviewed changes
src/gluonts/dataset/artificial/_base.py
@@ -84,7 +84,7 @@ def __init__(
self,
num_timeseries: int = 10,
num_steps: int = 30,
time_granularity: str = "1H",
freq: str = "1H",
Copy link

Choose a reason for hiding this comment

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

This needs to be kept backwards compatible in the json files for now. Use

freq: str = pydantic.Schema(..., alias='time_granularity', schema: '1H')

in order to allow use with existing metadata.json files.

Copy link

Choose a reason for hiding this comment

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

FYI you can use # type: ignore to silence mypy for this line.

@jaheba
Copy link
Contributor

jaheba commented Jul 4, 2019 •
edited

Can you check the env module?

(Especially the SageMakerEnv class.)

@lostella
Copy link
Contributor Author

lostella commented Jul 4, 2019

@jaheba I'll rebase this once #173 is merged

@aalexandrov
Copy link

@jaheba I'll rebase this once #173 is merged

@lostella let's do it the other way around.

@lostella lostella merged commit 2913a0c into awslabs:master Jul 4, 2019
@lostella lostella deleted the fix-freq branch July 4, 2019 15:23
aalexandrov pushed a commit to aalexandrov/gluon-ts that referenced this pull request Jul 4, 2019
Follow-up fixes of awslabs#172
20c4986 
- Allow backwards-compatible metadata.json files.
- Rename `time_freq` -> `freq`.
aalexandrov pushed a commit that referenced this pull request Jul 5, 2019
@aalexandrov
Follow-up fixes of #172
d70973f 
- Allow backwards-compatible metadata.json files.
- Rename `time_freq` -> `freq`.
@lostella lostella mentioned this pull request Jul 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalexandrov aalexandrov aalexandrov approved these changes

@geoalgo geoalgo geoalgo left review comments

Assignees
No one assigned
Labels
API change This item involves API change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lostella @szha @codecov-io @jaheba @aalexandrov @geoalgo