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

Implement Statistics.varm/stdm instead of Statistics._var #583

Merged
merged 4 commits into from
Dec 1, 2020
Merged

Implement Statistics.varm/stdm instead of Statistics._var #583

merged 4 commits into from
Dec 1, 2020

Conversation

sdewaele
Copy link
Contributor

This enables using StatsBase.mean_and_std / StatsBase.mean_and_std and consequently also ZScoreTransform.

See also JuliaStats/StatsBase.jl#622

@maleadt
Copy link
Member

maleadt commented Nov 30, 2020

Thanks!

This enables using StatsBase.mean_and_std / StatsBase.mean_and_std

Maybe add a test for those functions?

maleadt
maleadt reviewed Nov 30, 2020
View reviewed changes
Manifest.toml Outdated
@@ -111,7 +111,7 @@ deps = ["Libdl"]
uuid = "deac9b47-8bc7-5906-a0fe-35ac56dc84c0"

[[LibGit2]]
deps = ["NetworkOptions", "Printf"]
deps = ["Base64", "NetworkOptions", "Printf", "SHA"]
Copy link
Member

Choose a reason for hiding this comment

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

I take it this change is not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I'll change it back.

@maleadt maleadt added cuda array Stuff about CuArray. enhancement New feature or request labels Nov 30, 2020
@sdewaele
Copy link
Contributor Author

sdewaele commented Nov 30, 2020 •
edited

Thanks for your review!

I am currently testing the StatsBase GPU functionalty in the StatsBase test suite. fit(ZScoreTransform,...) uses StatsBase.mean_and_std under the hood.

In fact I had tests for these functions in a previous commit for this PR. I removed them to reduce the number of test dependencies for CUDA.jl, and also because StatsBase is maybe the logical home for these tests.

That said, I am happy to reintroduce them if you think it is better!

@maleadt
Copy link
Member

maleadt commented Nov 30, 2020

Ah OK, I misread StatsBase as Statistics. I agree it's better not to introduce yet another test dependency.

@codecov
Copy link

codecov bot commented Nov 30, 2020 •
edited

Codecov Report

Merging #583 (2858c64) into master (fc77b92) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #583   +/-   ##
=======================================
  Coverage   79.94%   79.94%           
=======================================
  Files         116      116           
  Lines        6870     6870           
=======================================
  Hits         5492     5492           
  Misses       1378     1378
Impacted Files Coverage Δ
src/statistics.jl 62.50% <50.00%> (-12.50%) ⬇️
lib/cudadrv/stream.jl 89.47% <0.00%> (+2.63%) ⬆️

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 fc77b92...2858c64. Read the comment docs.

@maleadt maleadt merged commit 53b9040 into JuliaGPU:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maleadt maleadt maleadt left review comments

Assignees
No one assigned
Labels
cuda array Stuff about CuArray. enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sdewaele @maleadt