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

Improve type inference of MixtureModel #1308

Merged
merged 6 commits into from
May 2, 2021
Merged

Conversation

devmotion
Copy link
Member

Fixes #1307 (or at least type inference of the functions that are mentioned in the issue, there might be some additional problems not covered by the tests).

The main issue was that the prior field is not concretely typed (Categorical{CT} is not a concrete type) and some hardcoded Float64 that broke type inference with dual numbers.

@devmotion devmotion requested a review from matbesancon April 21, 2021 13:51
Project.toml Outdated
@@ -1,7 +1,7 @@
name = "Distributions"
uuid = "31c24e10-a181-5473-b8eb-7969acd0382f"
authors = ["JuliaStats"]
version = "0.24.17"
version = "0.24.18"
Copy link
Member Author

Choose a reason for hiding this comment

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

What is the convention in Distributions - is the different type parameter of MixtureModel considered a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I updated the version number.

Copy link
Member Author

Choose a reason for hiding this comment

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

But now tests fail since there exists no version of HypothesisTests that is compatible with Distributions 0.25 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened a PR that removes the test dependency on HypothesisTests to fix these test failures: #1311

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

Merging #1308 (d51a7a6) into master (5604316) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1308      +/-   ##
==========================================
- Coverage   81.58%   81.48%   -0.11%     
==========================================
  Files         115      115              
  Lines        6659     6623      -36     
==========================================
- Hits         5433     5397      -36     
  Misses       1226     1226              
Impacted Files Coverage Δ
src/mixtures/mixturemodel.jl 68.11% <100.00%> (-4.73%) ⬇️

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 5604316...d51a7a6. Read the comment docs.

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

These are some nice simplifications. We'd have to bump the minor version to avoid breakage.

Project.toml Outdated
@@ -1,7 +1,7 @@
name = "Distributions"
uuid = "31c24e10-a181-5473-b8eb-7969acd0382f"
authors = ["JuliaStats"]
version = "0.24.17"
version = "0.24.18"
Copy link
Member

Choose a reason for hiding this comment

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

Yes

@matbesancon
Copy link
Member

@devmotion I think we should merge this, but merge any additional breaking change before tagging a new version

@matbesancon
Copy link
Member

Following a conversation re-started by @StefanKarpinski, we should probably then tag a 1.0

@devmotion
Copy link
Member Author

Following a conversation re-started by @StefanKarpinski, we should probably then tag a 1.0

I am not opposed to tagging a 1.0, but it wouldn't avoid the problem for downstream packages caused by breaking changes - we just have to bump the major version instead of the minor version.

@devmotion
Copy link
Member Author

any additional breaking change before tagging a new version

What other breaking changes are ready to be merged and released? Maybe #1286, or at least the bugfixes therein that could be extracted?

@matbesancon
Copy link
Member

#1286 is not breaking is it?

@devmotion
Copy link
Member Author

It changes the type of LocationScale, so it is breaking in the same way as this PR.

@devmotion
Copy link
Member Author

I guess it would also be a good opportunity to remove the deprecations in the upcoming breaking releases that have existed for quite some time.

@devmotion
Copy link
Member Author

How should we proceed? Merge #1286 and this PR and then tag 0.25?

@matbesancon
Copy link
Member

I would prefer keeping the deprecations and removing them when tagging 1.0, so yes let's go with these two PRs

@matbesancon
Copy link
Member

@devmotion can you merge master into your branch before we merge?

@devmotion
Copy link
Member Author

Done 🙂

@matbesancon matbesancon merged commit 99b2b57 into master May 2, 2021
@matbesancon matbesancon deleted the dw/mixture_inference branch May 2, 2021 08:36
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.

Type inference for mixture distributions
4 participants