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] Update Errors in docs: 10min-cudf-cupy.ipynb and Dataframes section #3618

Conversation

taureandyernv
Copy link
Contributor

@taureandyernv taureandyernv commented Dec 16, 2019

Solves #3617 by adding an ignore warnings line in the code.

Also solves #3638 dict syntax error in example

Currently, dlpack() warnings show up a couple of times in our documentation notebook https://rapidsai.github.io/projects/cudf/en/0.11.0/10min-cudf-cupy.html#

added warnings ignore code and removed warnings shown in our docs
@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #3618 into branch-0.12 will increase coverage by 2.16%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.12    #3618      +/-   ##
===============================================
+ Coverage        87.39%   89.55%   +2.16%     
===============================================
  Files               49       51       +2     
  Lines             9390    13293    +3903     
===============================================
+ Hits              8206    11904    +3698     
- Misses            1184     1389     +205
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 93.37% <ø> (+0.1%) ⬆️
python/cudf/cudf/core/dtypes.py 96% <0%> (-1.57%) ⬇️
python/cudf/cudf/utils/applyutils.py 100% <0%> (ø) ⬆️
python/cudf/cudf/core/table.py 100% <0%> (ø)
python/cudf/cudf/_libxx/__init__.py 100% <0%> (ø)
python/cudf/cudf/core/series.py 94.23% <0%> (+0.38%) ⬆️
python/dask_cudf/dask_cudf/backends.py 95.23% <0%> (+0.69%) ⬆️
python/dask_cudf/dask_cudf/core.py 71.02% <0%> (+0.93%) ⬆️
python/cudf/cudf/core/column/string.py 95.96% <0%> (+0.96%) ⬆️
... and 16 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 995c510...f42ea38. Read the comment docs.

@taureandyernv taureandyernv changed the title Update 10min-cudf-cupy.ipynb [WIP] Update Errors in docs: 10min-cudf-cupy.ipynb and Dataframes section Dec 18, 2019
fixed error in dict syntax
@taureandyernv taureandyernv requested a review from a team as a code owner December 18, 2019 18:04
@taureandyernv taureandyernv changed the title [WIP] Update Errors in docs: 10min-cudf-cupy.ipynb and Dataframes section [REVIEW] Update Errors in docs: 10min-cudf-cupy.ipynb and Dataframes section Dec 18, 2019
@cwharris
Copy link
Contributor

cwharris commented Dec 19, 2019

How come Codecov is reporting changes in test coverage? These all seem to be doc / notebook changes.

Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

LGTM

@jakirkham
Copy link
Member

@beckernick, would you be able to take a look at this? 🙂

@beckernick
Copy link
Member

beckernick commented Jan 6, 2020

Nice catch on the small dataframe docstring error. This LGTM also.

With that said, @taureandyernv those dlpack warnings are not superfluous. They are providing materially relevant information.

It might be nice to add a line indicating that cuDF will throw a warning as a reminder to check if you need to transpose your data before using the dlpack interface. I'd not go as far as saying it's necessary since transposing is discussed in the current text.

removed the removal of warnings
@taureandyernv
Copy link
Contributor Author

Nice catch on the small dataframe docstring error. This LGTM also.

With that said, @taureandyernv those dlpack warnings are not superfluous. They are providing materially relevant information.

It might be nice to add a line indicating that cuDF will throw a warning as a reminder to check if you need to transpose your data before using the dlpack interface. I'd not go as far as saying it's necessary since transposing is discussed in the current text.

@beckernick , great point to keep the warnings as that's the case. I changed the code to no longer ignore the warnings.

@kkraus14
Copy link
Collaborator

rerun tests

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge Python Affects Python cuDF API. doc Documentation labels Jan 20, 2020
@kkraus14 kkraus14 merged commit ed33cb8 into rapidsai:branch-0.12 Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge doc Documentation Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants