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

[REVIEW] Improved Array Conversion with CumlArrayDescriptor and Decorators [skip-ci] #3040

Merged

Conversation

mdemoret-nv
Copy link
Contributor

@mdemoret-nv mdemoret-nv commented Oct 22, 2020

Due to the large number of input and output array types that cuml supports, the library needs to be flexible in how array data is handed when entering and exiting the API. This has led to some inconsistent usage of CumlArray, unnecessary conversions and memcpys, and adds repeated work for the developers.

This PR helps with these issues by adding a new object CumlArrayDescriptor and a set of decorators in cuml.internals that make working with array data easier for developers and more consistent for users, while still being flexible enough to handle any scenario. Finally, this PR updates the majority of the code base to be consistent and use the new features.

A detailed description of the new features, how to use them, and whats going on internally, can be found in the new Estimator Guide.

Note: While the number of changed files is very large, the meat of the new features is limited to 2 sections. The rest of the changes are updates to use the new features and are very repetitive and formulaic.

mdemoret-nv and others added 30 commits July 28, 2020 09:47
python/cuml/test/conftest.py Outdated Show resolved Hide resolved
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.

Some minor comments, I have pretty much finished a first pass review

python/cuml/cluster/dbscan.pyx Outdated Show resolved Hide resolved
python/cuml/common/array_descriptor.py Outdated Show resolved Hide resolved
python/cuml/common/array_sparse.py Show resolved Hide resolved
python/cuml/common/base.py Outdated Show resolved Hide resolved
python/cuml/common/base.py Outdated Show resolved Hide resolved
python/cuml/linear_model/logistic_regression.pyx Outdated Show resolved Hide resolved
python/cuml/metrics/confusion_matrix.py Outdated Show resolved Hide resolved
python/cuml/svm/svm_base.pyx Outdated Show resolved Hide resolved
python/cuml/test/test_cuml_descr_decor.py Show resolved Hide resolved
python/cuml/tsa/holtwinters.pyx Outdated Show resolved Hide resolved
@mdemoret-nv mdemoret-nv changed the title [REVIEW] Improved Array Conversion with CumlArrayDescriptor and Decorators [skip-ci] [REVIEW] Improved Array Conversion with CumlArrayDescriptor and Decorators Nov 11, 2020
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looking good! Let's discuss merge timing tomorrow. Just small comments here.

python/cuml/common/base.py Outdated Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Outdated Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Outdated Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Outdated Show resolved Hide resolved
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.

Super small comments and questions, overall it looks great!

python/cuml/cluster/dbscan.pyx Outdated Show resolved Hide resolved
python/cuml/common/array_descriptor.py Show resolved Hide resolved
python/cuml/common/base.pyx Outdated Show resolved Hide resolved
python/cuml/datasets/regression.pyx Outdated Show resolved Hide resolved
python/cuml/fil/fil.pyx Show resolved Hide resolved
python/cuml/test/test_cuml_descr_decor.py Outdated Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Outdated Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Outdated Show resolved Hide resolved
wiki/python/ESTIMATOR_GUIDE.md Show resolved Hide resolved
@mdemoret-nv
Copy link
Contributor Author

rerun tests

@JohnZed JohnZed changed the title [REVIEW] Improved Array Conversion with CumlArrayDescriptor and Decorators [REVIEW] Improved Array Conversion with CumlArrayDescriptor and Decorators [skip-ci] Nov 13, 2020
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.

6 participants