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

Convert fp32 datasets to fp64 in ARIMA and AutoARIMA + update notebook to avoid deprecation warnings with positional parameters #4195

Merged
merged 5 commits into from
Sep 8, 2022
Merged

Convert fp32 datasets to fp64 in ARIMA and AutoARIMA + update notebook to avoid deprecation warnings with positional parameters #4195

merged 5 commits into from
Sep 8, 2022

Conversation

Nyrio
Copy link
Contributor

@Nyrio Nyrio commented Sep 7, 2021

No description provided.

@Nyrio
Convert fp32 datasets to fp64 in ARIMA and AutoARIMA with a warning +…
0a7cf03 
… update notebook to avoid deprecation warnings with positional parameters
@Nyrio Nyrio requested a review from a team as a code owner September 7, 2021 17:56
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Sep 7, 2021
@Nyrio Nyrio added 3 - Ready for Review Ready for review by team non-breaking Non-breaking change Notebook Issue or PR related to cuML notebook examples improvement Improvement / enhancement to an existing function labels Sep 7, 2021
dantegd
dantegd reviewed Sep 7, 2021
View reviewed changes
python/cuml/tsa/arima.pyx Outdated
# Get device array. Float64 only for now.
self.d_y, self.n_obs, self.batch_size, self.dtype \
= input_to_cuml_array(endog, check_dtype=np.float64)
= input_to_cuml_array(endog, convert_to_dtype=np.float64)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
= input_to_cuml_array(endog, convert_to_dtype=np.float64)
= input_to_cuml_array(
endog,
convert_to_dtype=(np.float64 if convert_dtype else None)
)

We can add a convert_dtype parameter in the __init__ (set by default to True) so that the user can disable the auto conversion if they want (similar to fit/predict in other models

cuml/python/cuml/linear_model/linear_regression.pyx

Line 222 in af13174

def fit(self, X, y, convert_dtype=True) -> "LinearRegression":
) what do you think?

Copy link
Contributor Author

@Nyrio Nyrio Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have added convert_dtype and removed the warning (unless I make False the default for this option, I don't think that it makes sense to have a warning, because this is the intended behavior of convert_dtype=True).

I guess we can implement a warning when we add float32 support.

@Nyrio Nyrio changed the title Convert fp32 datasets to fp64 in ARIMA and AutoARIMA with a warning + update notebook to avoid deprecation warnings with positional parameters Convert fp32 datasets to fp64 in ARIMA and AutoARIMA + update notebook to avoid deprecation warnings with positional parameters Sep 8, 2021
@Nyrio Nyrio added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Sep 10, 2021
@github-actions
Copy link

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

@github-actions
Copy link

This PR has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

@dantegd dantegd changed the base branch from branch-21.10 to branch-22.10 August 31, 2022 18:25
@codecov-commenter
Copy link

Codecov Report

Base: 78.02% // Head: 78.04% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (e850e62) compared to base (7a0ab85).
Patch coverage: 86.53% of modified lines in pull request are covered.

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-22.10    #4195      +/-   ##
================================================
+ Coverage         78.02%   78.04%   +0.02%     
================================================
  Files               180      180              
  Lines             11385    11423      +38     
================================================
+ Hits               8883     8915      +32     
- Misses             2502     2508       +6
Flag Coverage Δ
dask 46.29% <69.23%> (+0.07%) ⬆️
non-dask 67.32% <86.53%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
python/cuml/common/array.py 95.10% <85.10%> (-2.88%) ⬇️
python/cuml/cluster/__init__.py 100.00% <100.00%> (ø)
python/cuml/metrics/__init__.py 100.00% <100.00%> (ø)
python/cuml/thirdparty_adapters/adapters.py 91.54% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dantegd
Copy link
Member

dantegd commented Sep 8, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4b4c611 into rapidsai:branch-22.10 Sep 8, 2022
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
@Nyrio
Convert fp32 datasets to fp64 in ARIMA and AutoARIMA + update noteboo…
573772c 
…k to avoid deprecation warnings with positional parameters (rapidsai#4195)

Authors:
  - Louis Sugy (https://github.com/Nyrio)
  - Dante Gama Dessavre (https://github.com/dantegd)

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

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

@dantegd dantegd dantegd approved these changes

Assignees

@dantegd dantegd

Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function inactive-30d non-breaking Non-breaking change Notebook Issue or PR related to cuML notebook examples
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nyrio @codecov-commenter @dantegd