Add setitem method on EntitySet to overload add_dataframe method #1862

dvreed77 · 2022-01-24T14:52:32Z

Pull Request Description

Added a __setitem__ method on EntitySet to overload add_dataframe method
fixes Add dataframe with entityset setter #1685

…thod-overloads-add_dataframe

codecov · 2022-01-24T20:22:16Z

Codecov Report

Merging #1862 (ffbe8fd) into main (dfdd2b4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1862   +/-   ##
=======================================
  Coverage   98.82%   98.82%           
=======================================
  Files         145      145           
  Lines       16268    16277    +9     
=======================================
+ Hits        16077    16086    +9     
  Misses        191      191

Impacted Files	Coverage Δ
featuretools/entityset/entityset.py	`99.21% <100.00%> (+<0.01%)`	⬆️
featuretools/tests/entityset_tests/test_es.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfdd2b4...ffbe8fd. Read the comment docs.

thehomebrewnerd

I'm thinking we will want to update this guide in the documentation to discuss this approach to adding a dataframe to an EntitySet.

As part of this, we should mention the limitations of this approach, namely that if the dataframe does not have Woodwork initialized, the first column will be assumed to be the index column and all column types will be inferred by Woodwork. If users want to set the time index or have control over logical types, they should first initialize Woodwork on the dataframe with the proper parameters before assigning the dataframe to the EntitySet with this method.

rwedge · 2022-01-24T22:26:53Z

featuretools/tests/entityset_tests/test_es.py

@@ -8,6 +8,7 @@
 import numpy as np
 import pandas as pd
 import pytest
+from mock import patch


mock is part of the standard library now

from unittest.mock import patch

…thod-overloads-add_dataframe

thehomebrewnerd

Nothing major, just a couple things to clean up in release notes and a couple suggestions for the documentation.

thehomebrewnerd · 2022-01-26T15:19:53Z

docs/source/release_notes.rst

@@ -35,6 +35,7 @@ v1.4.0 Jan 10, 2022
        * Skip code coverage for specific dask usage lines (:pr:`1829`)
        * Increase minimum required numpy version to 1.21.0, scipy to 1.3.3, koalas to 1.8.1 (:pr:`1833`)
        * Remove pyyaml as a requirement (:pr:`1833`)
+        * Adds `__setitem__` method to overload `add_dataframe` method on EntitySet (:pr:`1862`)


This needs to move to the Future Release section above. Also, in order to get code formatting to work correctly, you will need to use double back-tics around the methods.

thehomebrewnerd · 2022-01-26T15:20:12Z

docs/source/release_notes.rst

@@ -45,7 +46,7 @@ v1.4.0 Jan 10, 2022
        * Remove fastparquet as a test requirement (:pr:`1833`)

    Thanks to the following people for contributing to this release:
-    :user:`davesque`, :user:`gsheni`, :user:`rwedge`, :user:`thehomebrewnerd`
+    :user:`davesque`, :user:`gsheni`, :user:`rwedge`, :user:`thehomebrewnerd`, :user:`dvreed77`


Move to Future Release instead

thehomebrewnerd · 2022-01-26T15:25:10Z

docs/source/getting_started/using_entitysets.ipynb

-    "* The ``logical_types`` parameter indicates that \"product_id\" should be interpreted as a Categorical column, even though it is just an integer in the underlying data."
+    "* The ``logical_types`` parameter indicates that \"product_id\" should be interpreted as a Categorical column, even though it is just an integer in the underlying data.\n",
+    "\n",
+    "You can also use a setter on the ``EntitySet`` object to add dataframes\n",


I think it might flow better if we move this a little lower in the section, after we demonstrate how to use add_dataframe.

Also, what do you think about putting this in a Note callout, like we do below with the description of plot?

I tried to put this into a note callout, but they don't support code blocks or lists. I guess I could reword it

Ah, ok, didn't realize that. No big deal on the note formatting, but if we can try somehow separate it from the flow of the example, that might be good.

Actually lists work, but can't get codeblocks to work. I reformatted it

thehomebrewnerd · 2022-01-26T15:25:57Z

docs/source/getting_started/using_entitysets.ipynb

+    "\n",
+    "* if the DataFrame does not have [Woodwork](https://woodwork.alteryx.com/) initialized, the first column will be the index column\n",
+    "* if the DataFrame does not have Woodwork initialized, all columns will be inferred by Woodwork.\n",
+    "* control over time index and logical types will need be initialized through Woodwork before adding."


Suggested rewording:
if control over the time index column and logical types is needed, Woodwork should be initialized before adding the dataframe

thehomebrewnerd

Looks good to me, but might be good to get someone else to review and approve before merging, just to make sure I'm not forgetting something that we wanted to do with this update.

tamargrey · 2022-01-26T20:44:57Z

docs/source/getting_started/using_entitysets.ipynb

+    "\n",
+    ".. note ::\n",
+    "\n",
+    "    You can also use a setter on the ``EntitySet`` object to add dataframes\n",


Repeat of the sentence above?

tamargrey · 2022-01-26T20:46:13Z

docs/source/getting_started/using_entitysets.ipynb

+    "    that this will use the default implementation of `add_dataframe`, notably the following:\n",
+    "\n",
+    "    * if the DataFrame does not have `Woodwork <https://woodwork.alteryx.com/>`_ initialized, the first column will be the index column\n",
+    "    * if the DataFrame does not have Woodwork initialized, all columns will be inferred by Woodwork.\n",


wording nitpick - can this say

all columns' logical types will be inferred by Woodwork

tamargrey · 2022-01-26T20:56:41Z

featuretools/entityset/entityset.py

@@ -671,6 +671,9 @@ def add_dataframe(self,
        self._add_references_to_metadata(dataframe)
        return self

+    def __setitem__(self, key, value):
+        self.add_dataframe(dataframe=value, dataframe_name=key)


Would it be worth handling the situation where a user passes in a woodwork-initialized dataframe that doesn't have a df.ww.name set (or if the df.ww.name is different from key) differently than add_dataframe does?

Currently the first situation would end up with an error Cannot add a Woodwork DataFrame to EntitySet without a name and the second raises a warning that the dataframe_name parameter is going to be ignored in favor of the name at df.ww.name.

I know there was some discussion with @thehomebrewnerd and @rwedge when we decided on that behavior for add_dataframe in the first place, but somehow setitem feels different to me. I know in woodwork's setitem, we just use the key value for a series' name even if we have to overwrite an already-existing name, and I think it might be work updating the woodwork name on the dataframe before calling add_dataframe.

@tamargrey: When looking at the behavior of add_dataframe when using a Woodwork initialized dataframe without a name, I do find it strange to get an error. I would think add_dataframe, when given a name, would add that name to the Woodwork dataframe. Maybe that behavior should change? I feel pretty strongly that __setitem__ should be a very thin layer over add_dataframe and call it with its default values, and let add_dataframe handle errors, warnings, etc.

Actually, a partially initialized Woodwork Dataframe causes all the issues that are described in the Note text, so maybe that is enough in this case

I think the first case should be handled by having add_dataframe add a name to woodwork-initialized dataframes that lack a name. There's a larger issue #1740 discussing it

Maybe @gsheni can chime in here, but I believe the underlying spirit of this ticket is to improve UX with Featuretools. So if I have an EntitySet and I just want to quickly add a dataframe (that hasn't been WW-initialized), I can just do es['dataframe'] = df. If the user wants more granular control, they can do that by using add_dataframe and additionally initializing the df using WW before adding it.

It'd be nice for this to also work with already woodworked dataframes but I'm fine with those edge cases being handled in other tickets

Yes, let's go for a good initial pass and then we can improve on it later on.

dvreed77 added 2 commits January 21, 2022 16:46

added setitem method to entityset

55deba7

trying to add test

101c471

auto-assign bot assigned dvreed77 Jan 24, 2022

dvreed77 added 4 commits January 24, 2022 09:56

Merge remote-tracking branch 'origin/main' into issue-1685-setitem-me…

8d4963a

…thod-overloads-add_dataframe

updated release notes

782919e

Merge remote-tracking branch 'origin/main' into issue-1685-setitem-me…

fe12590

…thod-overloads-add_dataframe

added test

1470b7c

dvreed77 requested review from thehomebrewnerd and gsheni January 24, 2022 20:23

dvreed77 changed the title ~~WIP: __setitem__ method overloads add_dataframe on EntitySet~~ __setitem__ method overloads add_dataframe on EntitySet Jan 24, 2022

gsheni requested review from rwedge and removed request for gsheni January 24, 2022 20:34

thehomebrewnerd reviewed Jan 24, 2022

View reviewed changes

rwedge reviewed Jan 24, 2022

View reviewed changes

gsheni changed the title ~~__setitem__ method overloads add_dataframe on EntitySet~~ Add __setitem__ method on EntitySet to overload add_dataframe method Jan 25, 2022

dvreed77 added 3 commits January 25, 2022 15:57

updated docs

66d85d8

fixed lint

208ee89

Merge remote-tracking branch 'origin/main' into issue-1685-setitem-me…

eeb5c9b

…thod-overloads-add_dataframe

dvreed77 requested review from thehomebrewnerd and rwedge January 26, 2022 14:51

thehomebrewnerd reviewed Jan 26, 2022

View reviewed changes

dvreed77 added 2 commits January 26, 2022 10:50

fixed documentation issues

d2f2d7b

fixed rst link

c7c8112

dvreed77 requested a review from thehomebrewnerd January 26, 2022 18:56

fixed docs

ffbe8fd

thehomebrewnerd approved these changes Jan 26, 2022

View reviewed changes

tamargrey reviewed Jan 26, 2022

View reviewed changes

dvreed77 merged commit 392c198 into main Jan 27, 2022

dvreed77 deleted the issue-1685-setitem-method-overloads-add_dataframe branch January 27, 2022 14:24

dvreed77 mentioned this pull request Feb 11, 2022

v1.5.0 #1901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setitem method on EntitySet to overload add_dataframe method #1862

Add setitem method on EntitySet to overload add_dataframe method #1862

dvreed77 commented Jan 24, 2022

codecov bot commented Jan 24, 2022 •

edited

Loading

thehomebrewnerd left a comment

rwedge Jan 24, 2022

thehomebrewnerd left a comment

thehomebrewnerd Jan 26, 2022

thehomebrewnerd Jan 26, 2022

thehomebrewnerd Jan 26, 2022

dvreed77 Jan 26, 2022

thehomebrewnerd Jan 26, 2022

dvreed77 Jan 26, 2022

thehomebrewnerd Jan 26, 2022

dvreed77 Jan 26, 2022

thehomebrewnerd left a comment

tamargrey Jan 26, 2022

tamargrey Jan 26, 2022

tamargrey Jan 26, 2022

dvreed77 Jan 26, 2022

dvreed77 Jan 26, 2022

rwedge Jan 26, 2022

dvreed77 Jan 26, 2022 •

edited by gsheni

Loading

rwedge Jan 26, 2022

gsheni Jan 26, 2022

Add __setitem__ method on EntitySet to overload add_dataframe method #1862

Add __setitem__ method on EntitySet to overload add_dataframe method #1862

Conversation

dvreed77 commented Jan 24, 2022

Pull Request Description

codecov bot commented Jan 24, 2022 • edited Loading

Codecov Report

thehomebrewnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehomebrewnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehomebrewnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvreed77 Jan 26, 2022 • edited by gsheni Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add setitem method on EntitySet to overload add_dataframe method #1862

Add setitem method on EntitySet to overload add_dataframe method #1862

codecov bot commented Jan 24, 2022 •

edited

Loading

dvreed77 Jan 26, 2022 •

edited by gsheni

Loading