-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix compatibility issues with Featuretools #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a release notes updated CI check in this repo, but we do have a release notes file, so it would be good to also update that with a comment before we merge.
autonormalize/autonormalize.py
Outdated
if time_index in current.df.columns: | ||
entities[current.index[0]] = (current.df, current.index[0], time_index) | ||
entities[current_df_name] = (current.df, current.index[0], time_index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have updated other libraries, we have moved away from using the entity/entities terminology in favor of dataframe/dataframes. If there are just a handful of places to update, maybe we should do that here now, but if there are a lot that need to be changed, we can create a new issue and work in it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! It wasn't too bad
autonormalize/autonormalize.py
Outdated
@@ -143,7 +150,7 @@ def auto_normalize(df): | |||
|
|||
def normalize_entity(es, accuracy=0.98): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want to update this from normalize_entity
to normalize_dataframe
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think it should be normalize_entityset
, but I didn't want to change the public API
docs/source/release_notes.rst
Outdated
* Changes | ||
* Rename `normalize_entity` to `normalize_entityset` (:pr:`41`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a breaking change to the API, we typically have been including a Breaking Change note in the release notes. Here is an example: https://github.com/alteryx/woodwork/blob/main/docs/source/release_notes.rst#breaking-changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fix compatibility issues with Featuretools
fixes #39