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

Remove static specifier in DecisionTree unit test for C++14 compliance #3281

Merged
merged 2 commits into from
Dec 17, 2020

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Dec 8, 2020

Replace "constexpr static" member variables in DecisionTree unit test fixture with "const" member variables for compliance with C++14, which otherwise requires that const static data members be separately defined in a namespace scope if it is ODR-used (See sections 3.2 and 9.4.2 of the C++11 standard, which remain relevant until C++17).

Replace "constexpr static" member variables in DecisionTree unit test
fixture with "const" member variables for compliance with C++14, which
otherwise requires that static data members be separately defined in a
namespace scope if it is ODR-used
@wphicks wphicks requested a review from a team as a code owner December 8, 2020 23:13
@wphicks wphicks added 3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change labels Dec 8, 2020
@wphicks
Copy link
Contributor Author

wphicks commented Dec 8, 2020

Together with PR #3279, this resolves #3212, allowing the debug build to compile and link correctly once more.

@wphicks
Copy link
Contributor Author

wphicks commented Dec 8, 2020

Oh, and for those wondering why this links at all in the non-debug build, it is because these are optimized as inline constants there.

@wphicks
Copy link
Contributor Author

wphicks commented Dec 9, 2020

The failures do not seem to be related to this change. The failures were in:

  • cuml.test.stemmer_tests.test_stemmer.test_same_results
  • cuml.test.stemmer_tests.test_steps.test_step5b

I'll rerun and if everything seems alright, I'll submit issues for those two.

@wphicks
Copy link
Contributor Author

wphicks commented Dec 9, 2020

rerun tests

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

lgtm

Bring in changes to avoid issue with stemmer test in CI
@wphicks
Copy link
Contributor Author

wphicks commented Dec 15, 2020

Merged in changes from 0.18 to eliminate failing test

@codecov-io
Copy link

codecov-io commented Dec 15, 2020

Codecov Report

Merging #3281 (4ddbd21) into branch-0.18 (2e4388d) will increase coverage by 0.23%.
The diff coverage is 92.85%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #3281      +/-   ##
===============================================
+ Coverage        71.45%   71.69%   +0.23%     
===============================================
  Files              205      207       +2     
  Lines            16594    16884     +290     
===============================================
+ Hits             11858    12105     +247     
- Misses            4736     4779      +43     
Impacted Files Coverage Δ
python/cuml/neighbors/__init__.py 100.00% <ø> (ø)
python/cuml/neighbors/ann.pxd 87.05% <87.05%> (ø)
python/cuml/common/sparsefuncs.py 91.66% <88.23%> (-2.34%) ⬇️
python/cuml/linear_model/base.pyx 97.36% <97.36%> (ø)
python/cuml/neighbors/nearest_neighbors.pyx 92.36% <98.24%> (+1.90%) ⬆️
python/cuml/linear_model/elastic_net.pyx 88.00% <100.00%> (-0.89%) ⬇️
python/cuml/linear_model/lasso.pyx 90.47% <100.00%> (-0.83%) ⬇️
python/cuml/linear_model/linear_regression.pyx 87.95% <100.00%> (-1.68%) ⬇️
python/cuml/linear_model/ridge.pyx 87.09% <100.00%> (-1.70%) ⬇️
python/cuml/manifold/t_sne.pyx 76.07% <100.00%> (+0.95%) ⬆️
... and 8 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 34efaf8...4ddbd21. Read the comment docs.

@wphicks
Copy link
Contributor Author

wphicks commented Dec 15, 2020

rerun tests

1 similar comment
@wphicks
Copy link
Contributor Author

wphicks commented Dec 17, 2020

rerun tests

@wphicks wphicks linked an issue Dec 17, 2020 that may be closed by this pull request
@rapids-bot rapids-bot bot merged commit ae7e444 into rapidsai:branch-0.18 Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Debug build results in compile error
3 participants