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

Support for all primitive types from array. #7003

Merged
merged 10 commits into from
Jun 1, 2021

Conversation

trivialfis
Copy link
Member

  • Add native support for CPU 128 float. cupy doesn't have float128, and CUDA treats long double as double in device code.
  • Convert boolean and float16 in Python.
  • The C API for constructing DMatrix from numpy array added in Support numpy array interface #6998 is renamed from Array to Dense for consistency.

Close #6999 .

* Add native support for CPU 128 float.
* Convert boolean and float16 in Python.
@trivialfis trivialfis requested review from hcho3 and RAMitchell May 27, 2021 11:52
src/data/array_interface.h Outdated Show resolved Hide resolved
@@ -378,3 +378,58 @@ def test_predict_dart(self, n_classes):

copied = cp.array(copied)
cp.testing.assert_allclose(inplace, copied, atol=1e-6)

def test_dtypes(self):
import cupy as cp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a pytestmark at the top of test_gpu_prediction.py to guard against the possibility that cupy isn't installed on the machine.

pytestmark = pytest.mark.skipif([cupy doesn't exist])

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. Added the predicate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The predicate should be added at the module level, since there are other test functions that also depends on cuPy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to some other functions. Some tests don't use cupy, so adding to module level seems overkill.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

Merging #7003 (ccb0c2c) into master (4cf95a6) will decrease coverage by 0.20%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7003      +/-   ##
==========================================
- Coverage   81.92%   81.71%   -0.21%     
==========================================
  Files          13       13              
  Lines        3911     3916       +5     
==========================================
- Hits         3204     3200       -4     
- Misses        707      716       +9     
Impacted Files Coverage Δ
python-package/xgboost/core.py 82.85% <33.33%> (-0.36%) ⬇️
python-package/xgboost/data.py 62.76% <42.85%> (+0.21%) ⬆️
python-package/xgboost/sklearn.py 89.49% <0.00%> (-1.04%) ⬇️
python-package/xgboost/dask.py 81.35% <0.00%> (-0.14%) ⬇️

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 4cf95a6...ccb0c2c. Read the comment docs.

@trivialfis
Copy link
Member Author

@hcho3 I have fixed the dask version on Travis for now due to dask/dask#7731 .

@trivialfis trivialfis merged commit ee4f51a into dmlc:master Jun 1, 2021
@trivialfis trivialfis deleted the all-dtypes branch June 1, 2021 00:34
@jrbourbeau
Copy link
Contributor

Apologies for the Dask breakages. We're working on a fix over in dask/dask#7743. Any feedback you may have on that PR is welcome.

@trivialfis
Copy link
Member Author

@jrbourbeau Thanks for informing us of the coming fix! I will pull the latest dask (or the PR branch) and test it tomorrow, hopefully that's not blocking your progress.

@jrbourbeau
Copy link
Contributor

That would be really useful, thanks @trivialfis : )

@trivialfis
Copy link
Member Author

@jrbourbeau Just tested the PR and it works great! Thanks for the fast response.

@jrbourbeau
Copy link
Contributor

🎉 thank you!

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.

libxgboost.so: undefined symbol: XGBoosterGetStrFeatureInfo
5 participants