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

add covariance_estimator #530

Closed
wants to merge 2 commits into from
Closed

add covariance_estimator #530

wants to merge 2 commits into from

Conversation

matthieugomez
Copy link
Contributor

@matthieugomez matthieugomez commented Oct 9, 2019

Return the CovarianceEstimator used to compute the variance-covariance matrix

Return the covariance_estimator used to compute the variance-covariance matrix
@nalimilan
Copy link
Member

Maybe we should just add a CovarianceEstimator method? Not sure. Cc: @kleinschmidt

@kleinschmidt
Copy link
Member

Does statsbase own CovarianceEstimator type?

@nalimilan
Copy link
Member

Yes:

CovarianceEstimator

@kleinschmidt
Copy link
Member

A few people on slack suggested that the constructor pun would be more appropriate when some kind of computation is involved, which (as far as I can tell) isn't the intended use case here, right? This would just retrieve an existing <:CovarianceEstimator. So I'm inclined to think that covariance_estimator seems more natural.

@matthieugomez
Copy link
Contributor Author

I'm personally indifferent, so let me know.

@nalimilan
Copy link
Member

Hard to decide... Another question if we decide to add a function: maybe it should be vcov_estimator since it goes with vcov?

src/statmodels.jl Outdated Show resolved Hide resolved
Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>
@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #530 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
- Coverage    90.5%   90.45%   -0.05%     
==========================================
  Files          21       21              
  Lines        2095     2096       +1     
==========================================
  Hits         1896     1896              
- Misses        199      200       +1
Impacted Files Coverage Δ
src/StatsBase.jl 100% <ø> (ø) ⬆️
src/statmodels.jl 49.54% <0%> (-0.46%) ⬇️

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 b039107...9e1a66f. Read the comment docs.

@matthieugomez
Copy link
Contributor Author

Or use covariance instead of vcov ;)

@nalimilan
Copy link
Member

Yeah, well, that would be breaking, so... An even more appealing solution would be to overload cov from Base, since that's really the same thing. Then you'd call the function covestimator, which is a bit shorter.

@matthieugomez
Copy link
Contributor Author

I don't think it's that important actually. Maybe we should close until we have a better story for covariance vcov etc?

@nalimilan
Copy link
Member

You mean you don't really need that function? Then yes, maybe better close until we have a use case, that's always better to design good APIs.

@matthieugomez
Copy link
Contributor Author

Exactly

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

3 participants