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

Improve FIL code readability and documentation #3056

Merged
merged 19 commits into from
Jun 30, 2021
Merged

Improve FIL code readability and documentation #3056

merged 19 commits into from
Jun 30, 2021

Conversation

levsnv
Copy link
Contributor

@levsnv levsnv commented Oct 23, 2020

@canonizer @hcho3 all proposals for more readable/simpler code welcome, especially feel free to push into this branch! :)

@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@levsnv levsnv mentioned this pull request Oct 23, 2020
Merged
@levsnv levsnv changed the base branch from branch-0.17 to branch-0.18 December 2, 2020 23:52
@github-actions
Copy link

This PR has been marked stale due to no recent activity in the past 30d. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be marked rotten if there is no activity in the next 60d.

@caryr35 caryr35 added this to PR-WIP in v21.06 Release via automation Apr 7, 2021
@wphicks
Copy link
Contributor

wphicks commented Apr 7, 2021

@levsnv Are we still hoping to get this in at some point, or is this all essentially outdated?

@levsnv levsnv changed the base branch from branch-0.18 to branch-0.20 April 8, 2021 05:32
@levsnv
Copy link
Contributor Author

levsnv commented Apr 8, 2021

@wphicks at some point, yes. It will not be hard to fix conflicts, and the fixes are mostly up to date AFAIK.
There is one "fix" I need to undo.

@caryr35 caryr35 removed this from PR-WIP in v21.06 Release Jun 4, 2021
@caryr35 caryr35 added this to PR-WIP in v21.08 Release via automation Jun 4, 2021
@levsnv levsnv changed the base branch from branch-21.06 to branch-21.08 June 5, 2021 03:55
@levsnv levsnv changed the title [WIP] kitchen sink for small readability changes to FIL (to avoid 100 PRs) Improve FIL code readability and documentation Jun 5, 2021
@levsnv levsnv requested a review from canonizer June 5, 2021 03:56
@levsnv levsnv added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Jun 5, 2021
canonizer
canonizer approved these changes Jun 7, 2021
View reviewed changes
Copy link
Contributor

@canonizer canonizer left a comment

Choose a reason for hiding this comment

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

Approved, provided that the comments are addressed.

python/cuml/fil/README.md Outdated Show resolved Hide resolved
python/cuml/fil/README.md Outdated Show resolved Hide resolved
@levsnv levsnv marked this pull request as ready for review June 21, 2021 19:59
@levsnv levsnv requested review from a team as code owners June 21, 2021 19:59
@levsnv levsnv added 3 - Ready for Review Ready for review by team and removed 4 - Waiting on Author Waiting for author to respond to review inactive-30d labels Jun 23, 2021
@levsnv
Copy link
Contributor Author

levsnv commented Jun 29, 2021

waiting for #4012 to rerun the CI

dantegd
dantegd requested changes Jun 29, 2021
View reviewed changes
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Just had one question before merging

python/cuml/fil/fil.pyx
""")
return func

def common_predict_params_docstring(func):
Copy link
Member

Choose a reason for hiding this comment

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

I think this is repeating functionality available here:

cuml/python/cuml/common/doc_utils.py

Line 352 in f71d369

def insert_into_docstring(parameters=False,

Was wondering if you tried using that existing decorator and if it caused any issues?

Copy link
Contributor Author

@levsnv levsnv Jun 29, 2021 •
edited

Choose a reason for hiding this comment

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

(sorry, misread the comment at first)
it wouldn't have For optimal performance, pass a device array with C-style layout.
generate_docstring couldn't handle

@generate_docstring
def f(X):
    """ """
    pass

so I used _parameters_docstrings to make it as close as possible

v21.08 Release automation moved this from PR-WIP to PR-Needs review Jun 29, 2021
@levsnv levsnv requested a review from dantegd June 30, 2021 07:59
@dantegd
Copy link
Member

dantegd commented Jun 30, 2021

rerun tests

v21.08 Release automation moved this from PR-Needs review to PR-Reviewer approved Jun 30, 2021
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@166667b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #3056   +/-   ##
===============================================
  Coverage                ?   85.47%           
===============================================
  Files                   ?      230           
  Lines                   ?    18130           
  Branches                ?        0           
===============================================
  Hits                    ?    15496           
  Misses                  ?     2634           
  Partials                ?        0
Flag Coverage Δ
dask 48.15% <0.00%> (?)
non-dask 77.75% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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 166667b...b1c8e8c. Read the comment docs.

@dantegd
Copy link
Member

dantegd commented Jun 30, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0744a2f into rapidsai:branch-21.08 Jun 30, 2021
v21.08 Release automation moved this from PR-Reviewer approved to Done Jun 30, 2021
@levsnv levsnv deleted the neater-code branch July 9, 2021 20:28
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@levsnv
Improve FIL code readability and documentation (rapidsai#3056)
d580da9 
@canonizer @hcho3 all proposals for more readable/simpler code welcome, especially feel free to push into this branch! :)

Authors:
  - https://github.com/levsnv

Approvers:
  - Andy Adinets (https://github.com/canonizer)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#3056
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@canonizer canonizer canonizer approved these changes

@dantegd dantegd dantegd approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team CUDA/C++ Cython / Python Cython or Python issue doc Documentation non-breaking Non-breaking change
Projects
No open projects
v21.08 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@levsnv @GPUtester @wphicks @dantegd @codecov-commenter @canonizer