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

feat!: Remove utils.is_categorical #108

Merged
merged 2 commits into from
Sep 6, 2023
Merged

Conversation

mbelak-dtml
Copy link
Collaborator

@mbelak-dtml mbelak-dtml commented Aug 30, 2023

Resolves #25

BREAKING CHANGE: function edvart.utils.is_categorical is removed. edvart.data_types.is_categorical
can be used instead, with similar behavior.
BREAKING CHANGE: Slightly changes behavior of column selection for some plots, e.g. parallel categories is now stricter in choosing which columns are considered as categorical.

@mbelak-dtml mbelak-dtml self-assigned this Aug 30, 2023
@mbelak-dtml mbelak-dtml force-pushed the feat/remove-utils-is-categorical branch 2 times, most recently from fc601b1 to 2150e77 Compare August 30, 2023 12:44
Preparatory commit for removal of `utils.is_categorical`. Slightly
changes behavior of some plots, e.g. parallel categories is now stricter in
choosing which columns are considered as categorical.
BREAKING CHANGE: function `edvart.utils.is_categorical` is removed. `edvart.data_types.is_categorical`
can be used instead, with similar behavior.

Resolves #25.
@mbelak-dtml mbelak-dtml force-pushed the feat/remove-utils-is-categorical branch from 2150e77 to 1633cc7 Compare August 30, 2023 12:51
@mbelak-dtml mbelak-dtml marked this pull request as ready for review August 30, 2023 13:20
@mbelak-dtml mbelak-dtml requested a review from lukany August 30, 2023 13:21
Copy link
Contributor

@lukany lukany left a comment

Choose a reason for hiding this comment

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

I think that the change in the first commit: "parallel categories is now stricter in
choosing which columns are considered as categorical" is actually a breaking. It's not breaking API change but the behaviour of edvart is different. I'd mention it as a breaking change in PR description.

Also,

BREAKING CHANGE: function `edvart.utils.is_categorical` is removed. `edvart.data_types.is_categorical`
can be used instead, with similar behavior.

should be in PR description, otherwise it won't be in changelog

@mbelak-dtml mbelak-dtml requested a review from lukany September 4, 2023 14:32
@mbelak-dtml
Copy link
Collaborator Author

I think that the change in the first commit: "parallel categories is now stricter in choosing which columns are considered as categorical" is actually a breaking. It's not breaking API change but the behaviour of edvart is different. I'd mention it as a breaking change in PR description.

Also,

BREAKING CHANGE: function `edvart.utils.is_categorical` is removed. `edvart.data_types.is_categorical`
can be used instead, with similar behavior.

should be in PR description, otherwise it won't be in changelog

Yup, added both breaking changes to the PR description.

@lukany lukany added this pull request to the merge queue Sep 6, 2023
Merged via the queue into main with commit 39df303 Sep 6, 2023
@lukany lukany deleted the feat/remove-utils-is-categorical branch September 6, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Function is_categorical is duplicated.
2 participants