-
Notifications
You must be signed in to change notification settings - Fork 890
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
Upgrade pandas to 1.1.0 and update .week usage #1094
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1094 +/- ##
==========================================
- Coverage 98.36% 98.35% -0.02%
==========================================
Files 126 126
Lines 13267 13275 +8
==========================================
+ Hits 13050 13056 +6
- Misses 217 219 +2
Continue to review full report at Codecov.
|
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.
@wsankey Thank you for the contribution! After some discussion, we have a different idea on how we would like to handle this situation for now so that we can suppress this warning without bumping the pandas minimum version.
It seems as if the current method of accessing the week will remain until pandas 2.0, so we can keep using our current code for the time being.
Here is an outline of what we would like to see:
- Keep the
week(vals)
function as it is currently: return vals.dt.week - Add a warnings filter immediately prior to the return statement to suppress the warning coming from Featuretools. The following code should work after importing
warnings
at the top of the file:
warnings.filterwarnings("ignore", message="Series.dt.weekofyear and Series.dt.week have been deprecated.", module="featuretools")
- Change the pandas version restriction to be
pandas>=0.24.1,<2.0.0
since we expect this code will break with version 2.0 - Update the changelog comment to better reflect these changes, and add your username to the list of contributors
We would like to include this in the next release of Featuretools, which we are planning to make later today. Sorry for the short notice, but do you think you would be able to make these updates today?
Thanks again!
docs/source/changelog.rst
Outdated
@@ -17,6 +17,7 @@ Changelog | |||
* Change default branch to ``main`` (:pr:`1038`) | |||
* Raise TypeError if improper input is supplied to ``Entity.delete_variables()`` (:pr:`1064`) | |||
* Updates for compatibility with pandas 1.1.0 (:pr:`1079`, :pr:`1089`) | |||
* Upgrade pandas to >=1.1.0 (:pr: `1094`) |
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.
Also, remove the space between :pr:
and 1094
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.
Ah that's what it was, thank you!
The message persists for me in the ipython terminal with this change. Changing the line to |
I think that change will cause any FutureWarning afterward to be suppressed - so if the user did something outside of featuretools that would generate a different FutureWarning, the would never see it. We were trying to limit scope to the specific message within featuretools. I tested from a command line script and it seemed to work, but didn't check in ipython. |
@wsankey when you were testing before were you defining the Week primitive in the ipython or importing it from the featuretools module? It's possible if you defined it in memory it did not get filtered due to the |
Understood. Yes that was the issue. Setting the specific warning message and importing the class resolved this issue. Thank you for the guidance @thehomebrewnerd and @rwedge! Updated PR to be limited in scope. |
We should probably add a test case to confirm the warning is being filtered out I believe this tests that a warning does not occur with pytest.warns(None) as record:
function()
assert not record @wsankey would you be willing to write a test case? |
Yes, sure @rwedge I just added a unit test. Not certain this is the proper spot but I manually checked it failed, then passed. |
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.
@wsankey thanks for the test case, that file is a good place for it.
Just a few small comments
docs/source/changelog.rst
Outdated
@@ -17,6 +17,7 @@ Changelog | |||
* Change default branch to ``main`` (:pr:`1038`) | |||
* Raise TypeError if improper input is supplied to ``Entity.delete_variables()`` (:pr:`1064`) | |||
* Updates for compatibility with pandas 1.1.0 (:pr:`1079`, :pr:`1089`) | |||
* Set pandas version to pandas>=0.24.1,<2.0.0. Filter week and weekofyear primitive warning. (:pr:`1094`) |
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.
Let's make this "Set pandas version to pandas>=0.24.1,<2.0.0. Filter pandas deprecation warning in Week primitive."
@@ -212,6 +214,7 @@ class Week(TransformPrimitive): | |||
|
|||
def get_function(self): | |||
def week(vals): | |||
warnings.filterwarnings("ignore", message="Series.dt.weekofyear and Series.dt.week have been deprecated.", module="featuretools") |
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.
this line is a little long (142), can we reduce line length
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.
👍
@@ -83,3 +83,14 @@ def test_age_nan(): | |||
ages = age(dates, time=datetime(2020, 2, 26)) | |||
correct_ages = [10.159, np.nan, 8.159] | |||
np.testing.assert_array_almost_equal(ages, correct_ages, decimal=3) | |||
|
|||
|
|||
def test_no_deprecation_message(): |
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.
let's call this "test_week_no_deprecation_message"
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.
👍
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! Thanks @wsankey for the contribution!
Thanks! Anyway to rerun these tests, I think it was a coincidence they failed.. |
OK thank you! |
Only the code coverage test failed and that was due to a known issue, wouldn't have been impacted by these code changes |
Pull Request Description
Addresses resolving the deprecation warning in the primitive cited in this issue: #1091. Tested via ipython (images below) and message is resolved.
Before
After
After creating the pull request: in order to pass the changelog_updated check you will need to update the "Future Release" section of
docs/source/changelog.rst
to include this pull request.