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.

Sign up for GitHub

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

Jump to bottom

Fix issues following update to pandas 1.0.1 #141

Merged
merged 6 commits into from Mar 27, 2020
Merged

Fix issues following update to pandas 1.0.1 #141

merged 6 commits into from Mar 27, 2020

Conversation

mesejo
Copy link
Contributor

@mesejo mesejo commented Mar 15, 2020

  • Change _stringify_path to stringify_path (make it public)
  • Fix info() formatting
  • Update output in notebooks cells and doctest strings
  • Keep the original dtype for some aggregate functions (min, max) by adding an optional parameter (keep_domain)

Closes #124

@mesejo
Fix the following problems after update to pandas 1.0.1
106fa81 
* Change _stringify_path to stringify_path (make it public)
* Keep the original dtype for some aggregate functions (min, max)
* Fix info() formatting
* Update output in notebooks cells and doctest strings
@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Mar 15, 2020 •
edited

💚 CLA has been signed

@sethmlarson
Copy link
Contributor

jenkins test this please

@mesejo
Copy link
Contributor Author

mesejo commented Mar 23, 2020 •
edited

@sethmlarson Thanks for triggering the build.

I took a look at the log, it is failing for Python 3.5,

14:42:50 No matching distribution found for pandas==1.0.1 (from -r requirements-dev.txt (line 2))
14:42:50 You are using pip version 9.0.1, however version 20.0.2 is available.

Currently the officially supported versions of Python for pandas 1.0.1 are 3.6.1, 3.7, 3.8. See here.

Hope this helps.

@sethmlarson
Copy link
Contributor

Yeah to support Pandas v1 we'd have to drop 3.5.3 which is probably fine. cc @stevedodson

@stevedodson
Copy link
Contributor

++ to mirror pandas 1.0.1 supported versions.

@sethmlarson
Copy link
Contributor

Okay, let's handle that in a separate PR since it'll be a much larger change than this one.

sethmlarson
sethmlarson suggested changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@sethmlarson sethmlarson left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for this change! Left your some comments.

I'll be opening a PR soon that drops Python 3.5 but also adds auto-formatting to the codebase.
This will certainly require you to do a merge commit which will probably be pretty ugly so plan for that, I can help if needed. It's also a good reason to try to keep this PR as minimal to pandas v1 as possible.

requirements-dev.txt Outdated Show resolved Hide resolved
eland/operations.py
@@ -109,10 +109,10 @@ def sum(self, query_compiler, numeric_only=True):
return self._metric_aggs(query_compiler, 'sum', numeric_only=numeric_only)

def max(self, query_compiler, numeric_only=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

From the above frames this keep_domain change looks pretty good, would also like @stevedodson to confirm here.

eland/dataframe.py Show resolved Hide resolved
@sethmlarson
Copy link
Contributor

sethmlarson commented Mar 27, 2020 •
edited

@mesejo Thanks for your patience, I've merged the PR that drops support for Python 3.5 (and introduces Black code style). You'll probably have a semi-nasty merge commit to resolve but would love to get this PR up-to-date with master and then reviewed :)

Thanks again!

@mesejo
Copy link
Contributor Author

mesejo commented Mar 27, 2020

@sethmlarson Hi! Should I do a rebase or just a merge will suffix?

@sethmlarson
Copy link
Contributor

@mesejo Merge will be fine, we'll squash the whole PR down at the end anyways! :)

@mesejo
Copy link
Contributor Author

mesejo commented Mar 27, 2020

@sethmlarson Good! Just another question, should I keep the pandas version as it is? I saw a comment of yours but I cannot find it now.

@sethmlarson
Copy link
Contributor

Yeah you should still change all mentions of pandas==0.25 to be pandas>=1. You can probably find with grep 'pandas==' -R . :)

sethmlarson
sethmlarson suggested changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Couple of comments!

eland/dataframe.py Outdated Show resolved Hide resolved
eland/dataframe.py Outdated Show resolved Hide resolved
eland/operations.py Show resolved Hide resolved
@sethmlarson
Copy link
Contributor

jenkins test this please

@sethmlarson
Copy link
Contributor

Looks like there are some issues with whitespace:

11:06:22 flake8 --ignore=E501,W503,E402,E712 setup.py noxfile.py eland/ docs/
11:06:24 eland/dataframe.py:253:75: W291 trailing whitespace
11:06:24 eland/dataframe.py:254:75: W291 trailing whitespace
11:06:24 eland/dataframe.py:255:75: W291 trailing whitespace
11:06:24 eland/dataframe.py:256:75: W291 trailing whitespace
11:06:24 eland/dataframe.py:257:75: W291 trailing whitespace
11:06:24 eland/dataframe.py:259:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:260:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:261:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:262:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:263:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:264:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:612:56: W291 trailing whitespace
11:06:24 eland/dataframe.py:613:56: W291 trailing whitespace

Try running nox -s lint

@sethmlarson
Copy link
Contributor

jenkins test this please

@mesejo
Copy link
Contributor Author

mesejo commented Mar 27, 2020

Hi @sethmlarson if I remove the trailing whitespaces then the doc-test will fail, the new format of info() has two trailing whitespaces.

@sethmlarson
Copy link
Contributor

sethmlarson commented Mar 27, 2020 •
edited

@mesejo That's unfortunate! Can you add W291 to the --ignore= within noxfile.py then? Whenever we have those issues it "should" be fixed by black automatically when there's trailing whitespace in code.

@sethmlarson
Copy link
Contributor

jenkins test this please

sethmlarson
sethmlarson approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Assuming CI passes this LGTM, thank you so much!

@sethmlarson
Copy link
Contributor

One question, are you okay with having your name mentioned in the release notes? We like to highlight community contributions and this one is a great example. :)

@mesejo
Copy link
Contributor Author

mesejo commented Mar 27, 2020

Yes I'm okay with that :)

@sethmlarson sethmlarson merged commit e27a508 into elastic:master Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sethmlarson sethmlarson sethmlarson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to pandas 1.0.1
4 participants
@mesejo @elasticmachine @sethmlarson @stevedodson