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.

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

REFACTOR-#4796: Introduce constant for __reduced__ column name #4799

Merged
merged 7 commits into from
Aug 10, 2022

Conversation

noloerino
Copy link
Collaborator

@noloerino noloerino commented Aug 9, 2022

Signed-off-by: Jonathan Shi jhshi@ponder.io

What do these changes do?

Adds the MODIN_UNNAMED_SERIES_LABEL to replace hardcoded instances of `"reduced" found throughout the codebase.

  • commit message follows format outlined here
  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves REFACTOR: Introduce constant for __reduced__ column names #4796
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date
  • added (Issue Number: PR title (PR Number)) and github username to release notes for next major release

…n name

Signed-off-by: Jonathan Shi <jhshi@ponder.io>
@noloerino noloerino requested review from a team as code owners August 9, 2022 00:32
Signed-off-by: Jonathan Shi <jhshi@ponder.io>
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #4799 (a46400a) into master (3e981ba) will increase coverage by 4.58%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4799      +/-   ##
==========================================
+ Coverage   85.28%   89.87%   +4.58%     
==========================================
  Files         259      260       +1     
  Lines       19381    19666     +285     
==========================================
+ Hits        16530    17674    +1144     
+ Misses       2851     1992     -859     
Impacted Files Coverage Δ
...frame/pandas/exchange/dataframe_protocol/column.py 76.36% <ø> (ø)
modin/pandas/groupby.py 93.71% <ø> (ø)
...n/core/dataframe/algebra/default2pandas/default.py 96.55% <100.00%> (ø)
...n/core/dataframe/algebra/default2pandas/groupby.py 96.50% <100.00%> (+0.02%) ⬆️
modin/core/dataframe/algebra/groupby.py 95.14% <100.00%> (ø)
modin/core/dataframe/pandas/dataframe/dataframe.py 94.33% <100.00%> (+<0.01%) ⬆️
modin/core/storage_formats/base/query_compiler.py 99.22% <100.00%> (+<0.01%) ⬆️
...odin/core/storage_formats/pandas/query_compiler.py 96.09% <100.00%> (ø)
...entations/omnisci_on_native/dataframe/dataframe.py 92.14% <100.00%> (+<0.01%) ⬆️
...tal/core/storage_formats/omnisci/query_compiler.py 94.49% <100.00%> (ø)
... and 46 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pyrito
pyrito previously approved these changes Aug 9, 2022
Copy link
Collaborator

@pyrito pyrito left a comment

Choose a reason for hiding this comment

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

Thank you, this seems more readable. What does __reduced__ even mean in this context?

@noloerino
Copy link
Collaborator Author

@pyrito The __reduced__ label is used for the internal query compiler representation of unnamed Series objects in order to distinguish between an N-element Series and a 1xN Dataframe (see relevant query compiler docs).

mvashishtha
mvashishtha previously approved these changes Aug 9, 2022
Copy link
Collaborator

@mvashishtha mvashishtha left a comment

Choose a reason for hiding this comment

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

LGTM @noloerino , thank you!

@mvashishtha mvashishtha dismissed stale reviews from pyrito and themself via 2cb8bea August 9, 2022 13:56
modin/utils.py Show resolved Hide resolved
pyrito
pyrito previously approved these changes Aug 9, 2022
Signed-off-by: Jonathan Shi <jhshi@ponder.io>
@noloerino noloerino dismissed stale reviews from pyrito and jeffreykennethli via dab9f63 August 9, 2022 20:28
pyrito
pyrito previously approved these changes Aug 10, 2022
Copy link
Collaborator

@mvashishtha mvashishtha left a comment

Choose a reason for hiding this comment

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

@noloerino I have a minor comment.

modin/utils.py Show resolved Hide resolved
mvashishtha
mvashishtha previously approved these changes Aug 10, 2022
@mvashishtha
Copy link
Collaborator

@prutskov @modin-project/modin-omnisci we need an omnisci approval.

Copy link
Contributor

@prutskov prutskov left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. But I found additional places in different parts of modin with direct using of __reduced__ string. We should change __reduced__->MODIN_UNNAMED_SERIES_LABEL everywhere (code/comments, docs, tests) to not worry about this places in the future in case we will want to change string value of MODIN_UNNAMED_SERIES_LABEL

Signed-off-by: Jonathan Shi <jhshi@ponder.io>
@noloerino noloerino dismissed stale reviews from mvashishtha and pyrito via 47e96f7 August 10, 2022 17:34
@noloerino
Copy link
Collaborator Author

@prutskov makes sense, I've replaced it in comments/docs as well.

Copy link
Contributor

@prutskov prutskov left a comment

Choose a reason for hiding this comment

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

Additional one minor suggestion

docs/release_notes/release_notes-0.16.0.rst Outdated Show resolved Hide resolved
Co-authored-by: Alexey Prutskov <lehaprutskov@gmail.com>
prutskov
prutskov previously approved these changes Aug 10, 2022
Copy link
Contributor

@prutskov prutskov left a comment

Choose a reason for hiding this comment

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

Thanks, @noloerino! LGTM

mvashishtha
mvashishtha previously approved these changes Aug 10, 2022
YarShev
YarShev previously approved these changes Aug 10, 2022
Copy link
Collaborator

@YarShev YarShev left a comment

Choose a reason for hiding this comment

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

LGTM, left a comment.

docs/flow/modin/core/storage_formats/index.rst Outdated Show resolved Hide resolved
@noloerino noloerino dismissed stale reviews from YarShev, mvashishtha, and prutskov via a46400a August 10, 2022 19:40
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
@YarShev YarShev merged commit 3f985ed into modin-project:master Aug 10, 2022
@noloerino noloerino deleted the reduced-constant branch August 23, 2022 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REFACTOR: Introduce constant for __reduced__ column names
7 participants