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

Rename get_sliced_child to sliced_child. #10881

Closed
wants to merge 1 commit into from

Conversation

bdice
Copy link
Contributor

@bdice bdice commented May 17, 2022

This PR renames the lists/structs methods get_sliced_child to sliced_child. This resolves some inconsistent naming (like structs_column_device_view::sliced_child). Resolves #10666.

@bdice bdice added code quality libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels May 17, 2022
@bdice bdice self-assigned this May 17, 2022
@github-actions github-actions bot added the Java Affects Java cuDF API. label May 17, 2022
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #10881 (bbae90a) into branch-22.06 (6352b4e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           branch-22.06   #10881      +/-   ##
================================================
+ Coverage         86.29%   86.33%   +0.03%     
================================================
  Files               144      144              
  Lines             22656    22665       +9     
================================================
+ Hits              19552    19567      +15     
+ Misses             3104     3098       -6     
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/groupby.py 97.42% <100.00%> (+<0.01%) ⬆️
python/dask_cudf/dask_cudf/tests/test_groupby.py 100.00% <100.00%> (ø)
python/cudf/cudf/core/dataframe.py 93.78% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.78% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.79% <0.00%> (+0.22%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 91.70% <0.00%> (+0.97%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80e4262...bbae90a. Read the comment docs.

@bdice bdice marked this pull request as ready for review May 18, 2022 03:18
@bdice bdice requested review from a team as code owners May 18, 2022 03:18
@bdice bdice requested review from robertmaynard and davidwendt May 18, 2022 03:18
@davidwendt
Copy link
Contributor

davidwendt commented May 18, 2022

So I'm not a big fan of this. I do think the get_ prefix is unnecessary for simple accessors or for retrieving virtual attributes of a class. But I prefer a verb prefix if the method must manufacture the result as in this case. The sliced child is not really an attribute of the class and so must be created on every call.
I mentioned virtual attribute because I know we have cases like null_count() which internally may require it to calculate the answer. But I think it is a reasonable expectation that it appear as a simple accessor. This is still not the case of a sliced child which I think does not meet this criteria as an expected attribute of the view.
If get is too controversial then perhaps make_sliced_child() or something similar.

@bdice
Copy link
Contributor Author

bdice commented May 18, 2022

@davidwendt I appreciate the critique -- my primary goal here is consistency with structs_column_device_view::sliced_child. I have heard we try to avoid get_ prefixes where possible, which led to this changeset, but I agree with your reasoning that this may be a different case than an accessor. Do you think it would be better to rename structs_column_device_view::sliced_child to structs_column_device_view::get_sliced_child?

@davidwendt
Copy link
Contributor

... I have heard we try to avoid get_ prefixes where possible, which led to this changeset, but I agree with your reasoning that this may be a different case than an accessor. Do you think it would be better to rename structs_column_device_view::sliced_child to structs_column_device_view::get_sliced_child?

Yes, I do.

@bdice
Copy link
Contributor Author

bdice commented May 18, 2022

Closing this in favor of #10885.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename structs_column_view::get_sliced_child
3 participants