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

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
@caryr35 caryr35 added this to PR-WIP in v21.10 Release via automation Sep 8, 2021
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v21.10 Release 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
@caryr35 caryr35 added this to PR-WIP in v21.12 Release via automation Oct 4, 2021
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v21.12 Release Oct 4, 2021
@caryr35 caryr35 removed this from PR-Needs review in v21.10 Release Oct 4, 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.

@caryr35 caryr35 added this to PR-WIP in v22.02 Release via automation Dec 15, 2021
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v22.02 Release Dec 15, 2021
@caryr35 caryr35 removed this from PR-Needs review in v21.12 Release Dec 15, 2021
@caryr35 caryr35 added this to PR-WIP in v22.04 Release via automation Feb 7, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v22.04 Release Feb 7, 2022
@caryr35 caryr35 removed this from PR-Needs review in v22.02 Release Feb 7, 2022
@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 removed this from PR-Needs review in v22.04 Release Mar 28, 2022
@dantegd dantegd added this to PR-WIP in v22.06 Release via automation Mar 28, 2022
@cjnolet cjnolet removed this from PR-WIP in v22.06 Release May 24, 2022
@cjnolet cjnolet added this to PR-WIP in v22.08 Release via automation May 24, 2022
@caryr35 caryr35 removed this from PR-WIP in v22.08 Release Aug 9, 2022
@caryr35 caryr35 added this to PR-WIP in v22.10 Release via automation Aug 9, 2022
@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.

v22.10 Release automation moved this from PR-WIP to PR-Reviewer approved Sep 8, 2022
@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
v22.10 Release automation moved this from PR-Reviewer approved to Done 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
No open projects
v22.10 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Nyrio @codecov-commenter @dantegd