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

Do not extended Statistics.cor/Statistics.cov #148

Merged
merged 1 commit into from
May 28, 2023
Merged

Conversation

giordano
Copy link
Member

Using the same functions as the Statistics ones is a pun, but the functions do different things, in particular they return scalar values, while the method defined here return matrices and make it impossible to do the same thing as the Statistics methods.

Also, adding the Statistics package as dependency with the sole purpose of extending methods of functions, which could have been done with an extension, increases loading time by a factor of 40.

Ref: f1d6e89#r115384906. CC: @LukasACH

Using the same functions as the `Statistics` ones is a pun, but the functions do
different things, in particular they return scalar values, while the method
defined here return matrices and make it impossible to do the same thing as the
`Statistics` methods.

Also, adding the `Statistics` package as dependency with the sole purpose of
extending methods of functions, which could have been done with an extension,
increases loading time by a factor of 40.
@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Merging #148 (fe65218) into master (628aad2) will not change coverage.
The diff coverage is 100.00%.

❗ Current head fe65218 differs from pull request most recent head 2d41312. Consider uploading reports for the commit 2d41312 to get more accurate results

@@           Coverage Diff           @@
##           master     #148   +/-   ##
=======================================
  Coverage   96.91%   96.91%           
=======================================
  Files           9        9           
  Lines         616      616           
=======================================
  Hits          597      597           
  Misses         19       19           
Impacted Files Coverage Δ
src/Measurements.jl 77.41% <ø> (ø)
src/utils.jl 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@LukasACH
Copy link
Contributor

I based my decision to extend Statistics on Distributions.jl which extends Statistics as well, and returns matrices for the cor and cov functions. But if it increases load times that much, it indeed does not make much sense.

@giordano
Copy link
Member Author

I based my decision to extend Statistics on Distributions.jl which extends Statistics as well, and returns matrices for the cor and cov functions

Distributions are their own entirely different datatype, Measurements are Numbers, and the correlation/covariance of vectors of numbers are already well-defined.

@giordano giordano merged commit c12fb10 into master May 28, 2023
12 checks passed
@giordano giordano deleted the mg/cov-cor branch May 28, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants