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

Fix promotion in cov and mean to handle integer and rational matrices #23285

Merged
merged 1 commit into from
Aug 18, 2017

Conversation

andreasnoack
Copy link
Member

Fixes #8080

scale!(unscaled_covzm(x, vardim), inv(size(x,vardim) - Int(corrected)))
function covzm(x::AbstractMatrix, vardim::Int=1; corrected::Bool=true)
C = unscaled_covzm(x, vardim)
T = typeof(first(C) / 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

can unscaled_covzm ever return heterogeneously-typed arrays?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can. Are you worried about first not being representative of the elements in C?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should usually be a subtype of the element type, but other elements might be wider for abstractly typed arrays, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that the element type has to be fairly concrete before we can instantiate it but I've just realized that in this case here, it should be possible to use promote_type(typeof(first(C) / 1), eltype(x)). I'll update.

Copy link
Contributor

Choose a reason for hiding this comment

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

eltype(x) or eltype(C)? will unscaled_covzm ever result in a different eltype?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably rare. I can only think of Bool entries and there is wouldn't change T but I think you are right that I should use C and not x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants