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

Revert "Add numerically safe ordered probit distribution." #4345

Closed
wants to merge 1 commit into from

Conversation

Spaak
Copy link
Member

@Spaak Spaak commented Dec 16, 2020

Reverts #4232 . Although all tests were fine within this PR branch, we now have a failure on master, see https://github.com/pymc-devs/pymc3/runs/1562295314 .

Any input on how to avoid such issues would be very helpful :) @MarcoGorelli perhaps? For now I think reverting the merge is probably the best option? (Which is what this PR does.)

Ping @tohtsky as well, who wrote #4232 . (Not to assign blame! You could not have foreseen this, given the OK test status on the PR.)

@michaelosthege
Copy link
Member

@Spaak I don't think we should revert. I had the same flaky test on #4214 but it went away after re-running the CI jobs.
So first just hit "re-run all jobs" on the master CI and it might turn green already.

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #4345 (e47c4c0) into master (9dbf9ea) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4345      +/-   ##
==========================================
+ Coverage   87.85%   87.87%   +0.01%     
==========================================
  Files          88       88              
  Lines       14501    14473      -28     
==========================================
- Hits        12740    12718      -22     
+ Misses       1761     1755       -6     
Impacted Files Coverage Δ
pymc3/distributions/__init__.py 100.00% <ø> (ø)
pymc3/distributions/discrete.py 96.78% <ø> (+1.56%) ⬆️
pymc3/distributions/dist_math.py 91.85% <ø> (-0.12%) ⬇️

@MarcoGorelli
Copy link
Contributor

MarcoGorelli commented Dec 16, 2020

So first just hit "re-run all jobs" on the master CI and it might turn green already.

Let's just make sure to leave some kind of comment/issue if we do this, flaky tests should be fixed eventually

@Spaak
Copy link
Member Author

Spaak commented Dec 16, 2020

Re-running CI on master now, let's hope the test failure indeed proves flaky and goes away. I'll leave this PR open until we know.

@Spaak
Copy link
Member Author

Spaak commented Dec 16, 2020

Yep, indeed CI on master is now all green. See #4323 for the issue about flaky tests.

@Spaak Spaak closed this Dec 16, 2020
@Spaak Spaak deleted the revert-4232-feature-oprobit branch December 16, 2020 11:38
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.

3 participants