Skip to content

Further fixes to time series module #1615

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
Merged
merged 10 commits into from
Sep 26, 2021
Merged

Further fixes to time series module #1615

merged 10 commits into from
Sep 26, 2021

Conversation

ngupta23
Copy link
Collaborator

Fixes:

#1555
#1563
#1613
#1565
#1591
Plus further fixes to plotting

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, local variables)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Unit tests were rerun

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

@ngupta23 ngupta23 added the time_series Topics related to the time series label Sep 26, 2021
@ngupta23 ngupta23 added this to the Time Series Beta Release milestone Sep 26, 2021
@ngupta23 ngupta23 requested a review from moezali1 September 26, 2021 15:34
@ngupta23 ngupta23 mentioned this pull request Sep 26, 2021
Closed
@satya-pattnaik
Copy link
Contributor

Is it safe to use Dict as a default argument?
Why is the empty dictionary a dangerous default value in python

@ngupta23
Copy link
Collaborator Author

ngupta23 commented Sep 26, 2021 •
edited
Loading

Is it safe to use Dict as a default argument?
Why is the empty dictionary a dangerous default value in python

@satya-pattnaik good catch. If you want to change the default to None and make the appropriate changes inside the functions, that would be OK with me. Would you be able to do that in your next PR?

@satya-pattnaik
Copy link
Contributor

Is it safe to use Dict as a default argument?
Why is the empty dictionary a dangerous default value in python

@satya-pattnaik good catch. If you want to change the default to None and make the appropriate changes inside the functions, that would be OK with me. Would you be able to do that in your next PR?

Yes, I was also thinking in the same line.

@ngupta23 ngupta23 merged commit 29df46a into time_series Sep 26, 2021
@ngupta23 ngupta23 deleted the ts_fixes_ng2 branch September 26, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yard1 Yard1 Yard1 approved these changes

@TremaMiguel TremaMiguel Awaiting requested review from TremaMiguel

@moezali1 moezali1 Awaiting requested review from moezali1

+1 more reviewer

@satya-pattnaik satya-pattnaik satya-pattnaik approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
time_series Topics related to the time series
Projects
None yet
Milestone
Time Series Beta Release
Development

Successfully merging this pull request may close these issues.
3 participants
@ngupta23 @satya-pattnaik @Yard1