Skip to content

Conversation

@torfjelde
Copy link
Member

@torfjelde torfjelde commented Jun 17, 2023

Attempt to fix tests as per TuringLang/DistributionsAD.jl#248

@devmotion
Copy link
Member

Shouldn't we also (even retro-actively) bound FillArrays in the Turing package itself to avoid that users run into these issues?

@torfjelde
Copy link
Member Author

Possibly! My worry about that is that:

  1. We're losing out on 1.1. and 1.2 just because of these somewhat specific scenarios. This doesn't seem to be an issue that occurs very widely.
  2. Could quickly run into an issue where one of our deps bump their compat bound because of some bug that was fixed after 1.0.0, in which case the user won't be able to stay on the most recent version of Turing.

IMO we don't for now, and then if we get people running into this issue in practice, we bound Turing too.

@devmotion
Copy link
Member

In my impression this FillArrays issue is a major problem as it affects (log) normal distributions (see, e.g., https://github.com/TuringLang/DistributionsAD.jl/actions/runs/5258560449/jobs/9502944457). I guess there are just not more test failures in Turing since we don't test as exhaustively as in DistributionsAD. Hence I think we should completely remove any use of FillArrays > 1.0.0 from the Turing ecosystem until these upstream issues are debugged and fixed.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2023

Pull Request Test Coverage Report for Build 5304666098

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 5278353164: 0.0%
Covered Lines: 0
Relevant Lines: 1427

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (6a307cd) 0.00% compared to head (66e75fb) 0.00%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2014   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1427    1427           
======================================
  Misses       1427    1427           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@torfjelde
Copy link
Member Author

Aight, let's do that then 👍

Comment on lines +222 to +223
# NOTE: Used to use `InverseGamma(2, 3)` but this has infinite variance
# which means that it's _very_ difficult to find a good tolerance in the test below:)
Copy link
Member Author

Choose a reason for hiding this comment

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

No wonder this test has been causing so much annoyance 😅

@torfjelde
Copy link
Member Author

Happy with this now @devmotion ?:)

@torfjelde torfjelde requested review from devmotion and yebai June 19, 2023 15:19
@torfjelde torfjelde merged commit 2dc6341 into master Jun 19, 2023
@torfjelde torfjelde deleted the torfjelde/test-fix branch June 19, 2023 15:22
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.

4 participants