[DataFrame] Implement rank by 11rohans · Pull Request #1991 · ray-project/ray

11rohans · 2018-05-03T05:06:53Z

Implements DataFrame.rank

AmplabJenkins · 2018-05-03T05:36:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5157/
Test FAILed.

AmplabJenkins · 2018-05-03T07:38:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5162/
Test PASSed.

AmplabJenkins · 2018-05-03T21:47:32Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5177/
Test PASSed.

kunalgosar

Looks really good. A few comments around naming, but otherwise looks good to go.

Rebase this on current master before it can be merged.

kunalgosar · 2018-05-03T23:39:38Z

python/ray/dataframe/dataframe.py

self.index?

kunalgosar · 2018-05-03T23:41:50Z

python/ray/dataframe/dataframe.py

s/remote_func/rank_helper/g

kunalgosar · 2018-05-03T23:43:02Z

python/ray/dataframe/test/test_dataframe.py

this doesn't call rank

fixed... lol

kunalgosar · 2018-05-03T23:43:32Z

python/ray/rllib/models/action_dist.py

revert changes to this file

p-yang

You should rebase your changes off of master since it's hard to tell which part is for this PR specfically.

p-yang · 2018-05-03T23:35:50Z

python/ray/dataframe/dataframe.py

You should put a more descriptive name since a lot of things are "remote functions"

p-yang · 2018-05-03T23:37:13Z

python/ray/dataframe/dataframe.py

self.index is more stylistically similar to the rest of the code, it should just be an alias for what you already have here

p-yang · 2018-05-03T23:39:42Z

python/ray/dataframe/dataframe.py

You should use _get_axis_number to generalize the axis names. Refer here for details.

AmplabJenkins · 2018-05-04T04:42:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5193/
Test FAILed.

kunalgosar

A few last comments.

kunalgosar · 2018-05-04T05:07:26Z

python/ray/dataframe/dataframe.py

NIT: this is not needed.

kunalgosar · 2018-05-04T05:08:17Z

python/ray/dataframe/dataframe.py

axis=None throws errors. Should not handle this case here.

In [3]: df.rank(axis=None) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) <ipython-input-3-7bd95fb0ea7b> in <module>() ----> 1 df.rank(axis=None) ~/dev/anaconda3/envs/ray-dev/lib/python3.5/site-packages/pandas/core/generic.py in rank(self, axis, method, numeric_only, na_option, ascending, pct) 5625 ranks : same type as caller 5626 """ -> 5627 axis = self._get_axis_number(axis) 5628 5629 if self.ndim > 2: ~/dev/anaconda3/envs/ray-dev/lib/python3.5/site-packages/pandas/core/generic.py in _get_axis_number(self, axis) 355 pass 356 raise ValueError('No axis named {0} for object type {1}' --> 357 .format(axis, type(self))) 358 359 def _get_axis_name(self, axis): ValueError: No axis named None for object type <class 'pandas.core.frame.DataFrame'>```

kunalgosar · 2018-05-04T05:08:28Z

python/ray/dataframe/test/test_dataframe.py

Should add tests for axis=1

AmplabJenkins · 2018-05-04T05:31:02Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5195/
Test PASSed.

kunalgosar

Looks great!

AmplabJenkins · 2018-05-04T20:46:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5225/
Test PASSed.

kunalgosar · 2018-05-06T04:23:43Z

This is currently failing tests on python 2 due to pandas-dev/pandas#20962

AmplabJenkins · 2018-05-06T05:26:20Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5234/
Test PASSed.

AmplabJenkins · 2018-05-06T06:40:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5236/
Test PASSed.

kunalgosar · 2018-05-06T06:48:47Z

If intending to merge, perhaps warn or error on python2 as behavior may not match expectations.

devin-petersohn · 2018-05-06T16:32:04Z

@kunalgosar Throwing a warning every time a user calls rank on Python2 will probably cause unnecessary concern. We do what Pandas documentation says it does, and since it is a bug in Pandas, we probably shouldn't worry too much about it. If people have a question they can open an issue and we'll just link to the Pandas issue.

devin-petersohn · 2018-05-06T16:32:44Z

Merged, thanks @11rohans!

* master: (21 commits) Expand local_dir in Trial init (ray-project#2013) Fixing ascii error for Python2 (ray-project#2009) [DataFrame] Implements df.update (ray-project#1997) [DataFrame] Implements df.as_matrix (ray-project#2001) [DataFrame] Implement quantile (ray-project#1992) [DataFrame] Impement sort_values and sort_index (ray-project#1977) [DataFrame] Implement rank (ray-project#1991) [DataFrame] Implemented prod, product, added test suite (ray-project#1994) [DataFrame] Implemented __setitem__, select_dtypes, and astype (ray-project#1941) [DataFrame] Implement diff (ray-project#1996) [DataFrame] Implemented nunique, skew (ray-project#1995) [DataFrame] Implements filter and dropna (ray-project#1959) [DataFrame] Implements df.pipe (ray-project#1999) [DataFrame] Apply() for Lists and Dicts (ray-project#1973) Clean up syntax for supported Python versions. (ray-project#1963) [DataFrame] Implements mode, to_datetime, and get_dummies (ray-project#1956) [DataFrame] Fix dtypes (ray-project#1930) keep_dims -> keepdims (ray-project#1980) add pthread linking (ray-project#1986) [DataFrame] Add layer of abstraction to allow OID instantiation (ray-project#1984) ...

* master: (25 commits) [DataFrame] Add direct pandas imports for MVP (ray-project#1960) Make ActorHandles pickleable, also make proper ActorHandle and ActorC… (ray-project#2007) Expand local_dir in Trial init (ray-project#2013) Fixing ascii error for Python2 (ray-project#2009) [DataFrame] Implements df.update (ray-project#1997) [DataFrame] Implements df.as_matrix (ray-project#2001) [DataFrame] Implement quantile (ray-project#1992) [DataFrame] Impement sort_values and sort_index (ray-project#1977) [DataFrame] Implement rank (ray-project#1991) [DataFrame] Implemented prod, product, added test suite (ray-project#1994) [DataFrame] Implemented __setitem__, select_dtypes, and astype (ray-project#1941) [DataFrame] Implement diff (ray-project#1996) [DataFrame] Implemented nunique, skew (ray-project#1995) [DataFrame] Implements filter and dropna (ray-project#1959) [DataFrame] Implements df.pipe (ray-project#1999) [DataFrame] Apply() for Lists and Dicts (ray-project#1973) Clean up syntax for supported Python versions. (ray-project#1963) [DataFrame] Implements mode, to_datetime, and get_dummies (ray-project#1956) [DataFrame] Fix dtypes (ray-project#1930) keep_dims -> keepdims (ray-project#1980) ...

* master: [DataFrame] Add direct pandas imports for MVP (ray-project#1960) Make ActorHandles pickleable, also make proper ActorHandle and ActorC… (ray-project#2007) Expand local_dir in Trial init (ray-project#2013) Fixing ascii error for Python2 (ray-project#2009) [DataFrame] Implements df.update (ray-project#1997) [DataFrame] Implements df.as_matrix (ray-project#2001) [DataFrame] Implement quantile (ray-project#1992) [DataFrame] Impement sort_values and sort_index (ray-project#1977) [DataFrame] Implement rank (ray-project#1991) [DataFrame] Implemented prod, product, added test suite (ray-project#1994) [DataFrame] Implemented __setitem__, select_dtypes, and astype (ray-project#1941) [DataFrame] Implement diff (ray-project#1996) [DataFrame] Implemented nunique, skew (ray-project#1995) [DataFrame] Implements filter and dropna (ray-project#1959) [DataFrame] Implements df.pipe (ray-project#1999) [DataFrame] Apply() for Lists and Dicts (ray-project#1973)

#19996) This reverts commit f1eedb1. ## Why are these changes needed? Self node should avoid reading any updates from gcs for node resource change since it'll maintain local view by itself. ## Related issue number #19438

11rohans changed the title ~~Ray rank~~ [DataFrame] Implement rank May 3, 2018

kunalgosar suggested changes May 3, 2018

View reviewed changes

p-yang reviewed May 3, 2018

View reviewed changes

11rohans force-pushed the ray_rank branch from 68536a9 to 561bcb3 Compare May 4, 2018 04:12

kunalgosar suggested changes May 4, 2018

View reviewed changes

kunalgosar approved these changes May 4, 2018

View reviewed changes

11rohans added 7 commits May 5, 2018 21:18

rank method completed

05d401b

added sanity checks

81a9b80

flake8

832ca67

updated sanity checks

10b5e76

flake8

4e653c2

updated sanity checks and style

5511aab

updated dtype logic

2baf317

devin-petersohn force-pushed the ray_rank branch from f8428a5 to 2baf317 Compare May 6, 2018 04:21

devin-petersohn approved these changes May 6, 2018

View reviewed changes

Fixing test

1a96257

devin-petersohn merged commit 9f28529 into ray-project:master May 6, 2018

Conversation

11rohans commented May 3, 2018

Uh oh!

AmplabJenkins commented May 3, 2018

Uh oh!

AmplabJenkins commented May 3, 2018

Uh oh!

AmplabJenkins commented May 3, 2018

Uh oh!

kunalgosar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-yang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 4, 2018

Uh oh!

kunalgosar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 4, 2018

Uh oh!

kunalgosar left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 4, 2018

Uh oh!

kunalgosar commented May 6, 2018

Uh oh!

AmplabJenkins commented May 6, 2018

Uh oh!

AmplabJenkins commented May 6, 2018

Uh oh!

kunalgosar commented May 6, 2018

Uh oh!

devin-petersohn commented May 6, 2018

Uh oh!

devin-petersohn commented May 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants