-
Notifications
You must be signed in to change notification settings - Fork 651
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
FEAT-#3303: Add __getitem__ for Resampler #3613
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3613 +/- ##
===========================================
- Coverage 85.47% 58.82% -26.65%
===========================================
Files 197 183 -14
Lines 16320 15661 -659
===========================================
- Hits 13949 9213 -4736
- Misses 2371 6448 +4077
Continue to review full report at Codecov.
|
This pull request introduces 1 alert when merging 78cb77d5cf5499b8aa1c2fc0ad367ca77e1711a9 into 7a81588 - view on LGTM.com new alerts:
|
78cb77d
to
6d37505
Compare
9b0f683
to
707129b
Compare
707129b
to
54792eb
Compare
@Rubtsowa, address existing comments please |
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.
Please, address the old comments. Also, you didn't fully changed the behavior requested in
Lines 3112 to 3113 in 6178a57
# FIXME: this should be converted into a dict to ensure simplicity | |
# of handling resample parameters at the query compiler level. |
But I don't mind if moving to dict using will be done in a separate PR as resolution of new issue (in that case create issue, please)
@modin-project/modin-core please take a look |
I created Issue #3724 |
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
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, left a minor comment
"tuple", | ||
"series", | ||
"index", | ||
"duplicated_column", |
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.
"duplicated_column", | |
"duplicate_column", |
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
…nal tests Signed-off-by: Alexey Prutskov <alexey.prutskov@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
Signed-off-by: Maria Rubtsova <maria.rubtsova@intel.com>
b4beef5
to
947aa9e
Compare
@modin-project/modin-core CI is green |
@vnlitvinov what do you think, are we good to go here or you have more comments on the 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.
Looks good with a potential code reduce which could be skipped.
subset = self._dataframe[key] | ||
resampler = type(self)(subset, *self.resample_args) | ||
return resampler |
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.
subset = self._dataframe[key] | |
resampler = type(self)(subset, *self.resample_args) | |
return resampler | |
return Resampler(self._dataframe[key], *self.resample_args) |
I don't see a point in doing 3 lines instead of one here, unless you expect this method to be inherited by some child class?.. I mean this type(self)(args)
thing looks cool but somewhat hacky.
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 might want to have a child class (like we have Dataframe(BasePandasDataset)) that inherits the method so I think it is fine.
Signed-off-by: Maria Rubtsova maria.rubtsova@intel.com
What do these changes do?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/developer/architecture.rst
is up-to-date