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

**BREAKING** Fix loss of structure in cov() and invcov() of AbstractMvNormal subtypes #1373

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

st--
Copy link
Contributor

@st-- st-- commented Jul 28, 2021

Resolves #572. Distributions.jl has a lower bound of PDMats=0.10, in which PDMat and PDiagMat are subtypes of AbstractMatrix, so no need to call Matrix() anymore.

What, if anything, do I still need to add to this PR to get it merged ?

@st-- st-- changed the title Keep structure in cov() and invcov() of AbstractMvNormal subtypes Fix loss of structure in cov() and invcov() of AbstractMvNormal subtypes Aug 2, 2021
@st--
Copy link
Contributor Author

st-- commented Aug 23, 2021

Hi, is there anything that would hold back merging this PR ? @devmotion ?

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7e232ca) 86.00% compared to head (f919388) 83.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
- Coverage   86.00%   83.88%   -2.13%     
==========================================
  Files         144      144              
  Lines        8646     7004    -1642     
==========================================
- Hits         7436     5875    -1561     
+ Misses       1210     1129      -81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devmotion
Copy link
Member

IMO this is a breaking change and therefore we should make a breaking release. Since people dislike breaking releases of Distributions (#1317), we should check if there are any other breaking changes that could be included in Distributions 0.26.

@devmotion devmotion changed the title Fix loss of structure in cov() and invcov() of AbstractMvNormal subtypes **BREAKING** Fix loss of structure in cov() and invcov() of AbstractMvNormal subtypes Aug 30, 2021
@devmotion
Copy link
Member

I wonder if it is possible to check if/how breaking this would be for downstream packages. Is there actually any package that relies on cov returning a Matrix? I would like to merge this PR but it's blocked since it's technically breaking. In my experience though (also with e.g. SpecialFunctions 2) for such central packages such a tiny breaking change means putting a lot of work on downstream developers and requiring manual compat updates in many packages, but if the breaking change only affects very few packages it might not be worth it and easier to make a non-breaking release and fix a handful of packages instead of > 200.

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.

why do cov and invcov call full?
3 participants