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

Created an enum class for allowed time series plot data types and replaced string usages with TSAllowedPlotDataTypes Enum instances #2655

Merged
merged 8 commits into from Jun 26, 2022

Conversation

desaizeeshan22
Copy link
Contributor

@desaizeeshan22 desaizeeshan22 commented Jun 12, 2022

2415

Add ENUM for allowed data types instead of strings #2415

Closes #2415

Describe the changes you've made

Created an enum class for allowed time series plot data types and replaced string usages with TSAllowedPlotDataTypes Enum instances

Type of change

  • My code follows the code style of this project.
  • Code style update (formatting, local variables)

How Has This Been Tested?

Used the existing test_get_data_types_to_plot in the file test_time_series_utils_plots.py

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

NA

Checklist:

  • My code follows the code style of this project.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.

Screenshots

Original Updated
image image
image image
image image

moezali1 and others added 2 commits June 10, 2022 10:47
@ngupta23 ngupta23 changed the base branch from master to develop June 13, 2022 13:46
@ngupta23
Copy link
Collaborator

@desaizeeshan22 Thanks for taking the initiative to add this. Can you comment on the actual Issue so that I can assign it to you?

Also, a few changes - Currently, we are adding change to develop branch (this may change in the future to master but for now, it is develop. I have made the change in this PR, but the tests are failing now. Can you have a look please?

I see you have changed the allowed type to ENUM. Can you check if the plots still work, since the users will be specifying the string values in plot_model and check_stats not the ENUM.

eda.plot_model(data_kwargs={"plot_data_type": ["original", "imputed", "transformed"]) 

@desaizeeshan22
Copy link
Contributor Author

Ohh okay I see the plots are failing because the supported types are ENUMS.How do you suggest I go about changing this should I use the ENUM.value instead which outputs a string?

@ngupta23
Copy link
Collaborator

Ohh okay I see the plots are failing because the supported types are ENUMS.How do you suggest I go about changing this should I use the ENUM.value instead which outputs a string?

The user should still enter the string (we should not change the user interface). Inside the code, when you check for correctness, you will have to check the strong against the possible ENUM values (currently, it is just checking string against string).

@desaizeeshan22
Copy link
Contributor Author

desaizeeshan22 commented Jun 13, 2022

image

Can you check now ? The plots seem to be working now!

@ngupta23
Copy link
Collaborator

It seems that the tests are failing. Can you merge the latest develop branch into your branch and then we can evaluate the changes?

@desaizeeshan22
Copy link
Contributor Author

Yup just finished merging the latest changes on develop branch with my branch

@ngupta23 ngupta23 merged commit 768f2dc into pycaret:develop Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Add ENUM for allowed data types instead of strings
3 participants