Make `_get_tags` a class/static method #3257

dantegd · 2020-12-04T18:13:54Z

From suggestion and talks with @mdemoret-nv we came to the conclusion that the _get_tags function is very useful as a class/static method, so this PR changes it.

If we require tags that are instance based, we can extend/modify the method in the future, but for now this seems like the best balance of simplicity and usefulness.

…ds in inheriting estimators

GPUtester · 2020-12-04T18:14:23Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

mdemoret-nv

Changes look good. Only one minor suggestion to add an assertion to the tags test to make sure new methods going forward aren't instance or class methods.

mdemoret-nv · 2020-12-04T19:46:58Z

python/cuml/test/test_api.py

-    # mod = models[model_name]
-    # assert hasattr('_get_tags', m)

-    # for tag in tags:
-    #     assert
+    assert hasattr(model, '_get_tags')

-    print(model)
-    if model in (cuml.tsa.auto_arima.AutoARIMA, cuml.tsa.arima.ARIMA,
-                 cuml.tsa.holtwinters.ExponentialSmoothing):
-        mod = model(cp.ones(10))
-    else:
-        mod = model()
-
-    assert hasattr(mod, '_get_tags')
-
-    model_tags = mod._get_tags()


Immediately you can see how much better the testing will be with static tags

python/cuml/common/base.pyx

python/cuml/test/test_api.py

Co-authored-by: Michael Demoret <42954918+mdemoret-nv@users.noreply.github.com>

…atic-tags

dantegd · 2020-12-09T19:41:18Z

rerun tests

…17-static-tags

…base

…mators

dantegd · 2021-01-20T00:31:01Z

rerun tests

dantegd · 2021-02-03T03:21:53Z

@mdemoret-nv yeah was just busy debugging other things, will push the changes you request early tomorrow and ping you for a re-review, thanks!

…dparty module compliance

Co-authored-by: Michael Demoret <42954918+mdemoret-nv@users.noreply.github.com>

…ests happy

mdemoret-nv

Looks good to me. I think this will be useful to get merged. We can start reworking some of the hard-coded tests to use the mixins.

python/cuml/common/mixins.py

mdemoret-nv · 2021-02-23T23:46:17Z

python/cuml/test/test_api.py

+    assert static_tags['preferred_input_order'] == 'F'
+    assert dynamic_tags['preferred_input_order'] == 'F'


In this case, wouldnt it be better to error out since this clearly would be an error? Or are you testing this for overriding purposes?

Yeah, this test to be sure that the closest thing to the class in the MRO overwrites the farthest ones, added a quick comment to separate both tests and a quick note on the estimator guide about the MRO resolution and how that can affect tags.

codecov-io · 2021-02-24T19:58:08Z

Codecov Report

Merging #3257 (b2c9075) into branch-0.19 (39c7262) will increase coverage by 9.05%.
The diff coverage is 82.91%.

@@               Coverage Diff               @@
##           branch-0.19    #3257      +/-   ##
===============================================
+ Coverage        71.77%   80.83%   +9.05%     
===============================================
  Files              212      226      +14     
  Lines            17075    17705     +630     
===============================================
+ Hits             12256    14311    +2055     
+ Misses            4819     3394    -1425

Flag	Coverage Δ
dask	`45.21% <56.61%> (?)`
non-dask	`73.07% <81.89%> (?)`

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

Impacted Files	Coverage Δ
python/cuml/experimental/explainer/common.py	`88.05% <42.85%> (-4.01%)`	⬇️
python/cuml/neighbors/ann.pyx	`61.62% <61.62%> (ø)`
python/cuml/common/import_utils.py	`59.43% <66.66%> (+3.43%)`	⬆️
python/cuml/experimental/explainer/base.pyx	`67.06% <67.06%> (ø)`
...l/_thirdparty/sklearn/preprocessing/_imputation.py	`62.80% <80.00%> (+0.29%)`	⬆️
python/cuml/neighbors/nearest_neighbors.pyx	`92.40% <80.00%> (-0.31%)`	⬇️
python/cuml/dask/common/utils.py	`43.68% <83.33%> (+16.13%)`	⬆️
python/cuml/common/mixins.py	`95.45% <95.45%> (ø)`
...on/cuml/_thirdparty/sklearn/preprocessing/_data.py	`63.24% <100.00%> (-0.30%)`	⬇️
python/cuml/cluster/dbscan.pyx	`100.00% <100.00%> (ø)`
... and 108 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 8e497d0...b2c9075. Read the comment docs.

dantegd · 2021-02-24T20:38:36Z

@gpucibot merge

ENH Make _get_tags a class method in base and _more_tags static metho…

eec7270

…ds in inheriting estimators

dantegd added 2 - In Progress Currenty a work in progress Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 4, 2020

dantegd changed the title ~~Make _get_tags a class/static method~~ Make _get_tags a class/static method Dec 4, 2020

mdemoret-nv approved these changes Dec 4, 2020

View reviewed changes

dantegd mentioned this pull request Dec 7, 2020

[REVIEW] cuML's estimator Base class for preprocessing models #3270

Merged

dantegd and others added 4 commits December 7, 2020 15:27

FIX remove unnecessary check of class

d20da31

ENH add check of static _more_tags methods in inheritting classes

a77ce54

Co-authored-by: Michael Demoret <42954918+mdemoret-nv@users.noreply.github.com>

Merge branch '017-static-tags' of github.com:dantegd/cuml into 017-st…

831ca5b

…atic-tags

FIX Change dtype to avoid random forest warning

af5ae26

dantegd added 2 commits December 9, 2020 14:40

Merge branch 'branch-0.18' of https://github.com/rapidsai/cuml into 0…

ead0874

…17-static-tags

ENH Add proposal of static and instance _get_tags method

c56f0ca

dantegd changed the title ~~Make _get_tags a class/static method~~ Make _get_tags a class/static method [skip-ci] Dec 9, 2020

dantegd added the proposal Change current process or code label Dec 9, 2020

dantegd mentioned this pull request Jan 11, 2021

[BUG] Tag system #3333

Closed

dantegd added 4 commits January 19, 2021 15:45

Merge branch-0.18 into 017-static-tags

90ad45e

FEA Add input order mixins and move all mixins to separate file from …

5df730e

…base

FEA update all static tags before dynamic tags of the mro of the esti…

d31fc70

…mators

FEA Update all estimators to use tag mixins

22aaa1b

dantegd changed the title ~~Make _get_tags a class/static method [skip-ci]~~ Make _get_tags a class/static method Jan 20, 2021

FEA Update estimator guide

4ca8cc1

dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 2 - In Progress Currenty a work in progress labels Jan 20, 2021

dantegd marked this pull request as ready for review January 20, 2021 17:21

dantegd changed the base branch from branch-0.18 to branch-0.19 February 9, 2021 19:28

dantegd changed the title ~~Make _get_tags a class/static method~~ Make _get_tags a class/static method [skip-ci] Feb 9, 2021

dantegd added 2 commits February 9, 2021 14:19

Merge branch-0.18 into 017-static-tags

ac087f6

ENH Multiple enhancements and tests

91dfb71

JohnZed mentioned this pull request Feb 16, 2021

[FEA] Preprocessing module out of experimental #3483

Closed

10 tasks

dantegd added 3 commits February 19, 2021 19:48

ENH Refactor tag functionality into mixin, test improvements and thir…

dbc4b58

…dparty module compliance

ENH finish all tests

df4f82c

Merge remote-tracking branch 'original/branch-0.19' into 019-static-tags

f37bf72

dantegd changed the title ~~Make _get_tags a class/static method [skip-ci]~~ Make _get_tags a class/static method Feb 22, 2021

dantegd changed the title ~~Make _get_tags a class/static method~~ Make _get_tags a class/static method [skip-ci] Feb 22, 2021

dantegd and others added 4 commits February 22, 2021 10:43

Update wiki/python/ESTIMATOR_GUIDE.md

76175dd

Co-authored-by: Michael Demoret <42954918+mdemoret-nv@users.noreply.github.com>

Update wiki/python/ESTIMATOR_GUIDE.md

66ebef1

Co-authored-by: Michael Demoret <42954918+mdemoret-nv@users.noreply.github.com>

FIX PR review feedback

8bc2f39

FIX Copyright year

77299b3

dantegd changed the title ~~Make _get_tags a class/static method [skip-ci]~~ Make _get_tags a class/static method Feb 22, 2021

dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Feb 22, 2021

dantegd added 2 commits February 22, 2021 11:37

FIX Copyright year

d807422

FIX Add parameters to dummy estimators to make base class and clone t…

f307d38

…ests happy

mdemoret-nv approved these changes Feb 23, 2021

View reviewed changes

FIX PR review feedback

b2c9075

rapids-bot bot merged commit 82e58d6 into rapidsai:branch-0.19 Feb 24, 2021

dantegd mentioned this pull request Mar 7, 2021

[FEA] Add _estimator_type attribute to all clustering estimators #3462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `_get_tags` a class/static method #3257

Make `_get_tags` a class/static method #3257

dantegd commented Dec 4, 2020

GPUtester commented Dec 4, 2020

mdemoret-nv left a comment

mdemoret-nv Dec 4, 2020

dantegd commented Dec 9, 2020

dantegd commented Jan 20, 2021

dantegd commented Feb 3, 2021

mdemoret-nv left a comment

mdemoret-nv Feb 23, 2021

dantegd Feb 24, 2021

codecov-io commented Feb 24, 2021

dantegd commented Feb 24, 2021

		assert static_tags['preferred_input_order'] == 'F'
		assert dynamic_tags['preferred_input_order'] == 'F'

Make _get_tags a class/static method #3257

Make _get_tags a class/static method #3257

Conversation

dantegd commented Dec 4, 2020

GPUtester commented Dec 4, 2020

mdemoret-nv left a comment

Choose a reason for hiding this comment

mdemoret-nv Dec 4, 2020

Choose a reason for hiding this comment

dantegd commented Dec 9, 2020

dantegd commented Jan 20, 2021

dantegd commented Feb 3, 2021

mdemoret-nv left a comment

Choose a reason for hiding this comment

mdemoret-nv Feb 23, 2021

Choose a reason for hiding this comment

dantegd Feb 24, 2021

Choose a reason for hiding this comment

codecov-io commented Feb 24, 2021

Codecov Report

dantegd commented Feb 24, 2021

Make `_get_tags` a class/static method #3257

Make `_get_tags` a class/static method #3257