Skip to content

fix: time series model types available #1354

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 4 commits into from
Jun 18, 2021
Merged

fix: time series model types available #1354

merged 4 commits into from
Jun 18, 2021

Conversation

TremaMiguel
Copy link
Contributor

Describe the changes you've made

Major changes include:

  1. baseline, classical, linear, neighbors and tree model types available when calling models(). link
  2. Cast metrics name to upper case to ensure highlighting minimum value in compare_models(). link

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

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.

Screenshots

highlight_fix

@TremaMiguel TremaMiguel requested review from Yard1 and ngupta23 June 13, 2021 17:22
@Yard1
Copy link
Member

Yard1 commented Jun 14, 2021

@TremaMiguel Could we actually use an Enum for this instead?

@TremaMiguel
Copy link
Contributor Author

@Yard1 you mean replacing the variable MODEL_TYPES with an ENUM class ?, what would be the benefit here ?

@ngupta23
Copy link
Collaborator

@Yard1 you mean replacing the variable MODEL_TYPES with an ENUM class ?, what would be the benefit here ?

@TremaMiguel It is useful when you have categorical variables like this (similar to the FREQUENCY values that you have). ENUM comes with several benefits like easy edits, containment tests (allowed value test), etc.

@Yard1
Copy link
Member

Yard1 commented Jun 15, 2021

To add to what @ngupta23 said, Enum would have built-in checking for whether the values are allowed, and would allow us to use Intellisense when adding new models.

@TremaMiguel
Copy link
Contributor Author

Thanks for the references @Yard1 , @ngupta23 . I'll take a look at them and make appropriate changes.

ngupta23
ngupta23 reviewed Jun 18, 2021
View reviewed changes
@ngupta23 ngupta23 self-requested a review June 18, 2021 11:09
Yard1
Yard1 approved these changes Jun 18, 2021
View reviewed changes
Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

I've taken the liberty of using the Enum features more, let me know if anything is unclear @TremaMiguel

@TremaMiguel
Copy link
Contributor Author

Thanks @Yard1

@TremaMiguel TremaMiguel merged commit 0e94354 into time_series Jun 18, 2021
@Yard1 Yard1 deleted the time_series_fixes branch July 7, 2021 00:30
@ngupta23 ngupta23 added time_series Topics related to the time series and removed time_series_dev labels Sep 19, 2021
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

@ngupta23 ngupta23 ngupta23 approved these changes

Assignees
No one assigned
Labels
time_series Topics related to the time series
Projects
None yet
Milestone
Time Series Alpha Release
Development

Successfully merging this pull request may close these issues.
3 participants
@TremaMiguel @Yard1 @ngupta23