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

Add a Horseshoe prior Gibbs sampler #96

Merged
merged 1 commit into from
Nov 8, 2021

Conversation

xjing76
Copy link
Contributor

@xjing76 xjing76 commented Oct 4, 2021

Adding in Horsehoes gibbs and tests

@xjing76 xjing76 force-pushed the gibbshs branch 2 times, most recently from a815080 to 602ae4c Compare October 5, 2021 15:02
@xjing76 xjing76 force-pushed the gibbshs branch 7 times, most recently from c480a61 to fd288a9 Compare October 13, 2021 11:05
@xjing76
Copy link
Contributor Author

xjing76 commented Oct 13, 2021

@brandonwillard I just added the test cases for pm.Deterministic But when I am testing for that case it seems like within the step function the point (samples for beta)would be of wrong size. Could you take a look at this. I couldn't figure out why

@xjing76
Copy link
Contributor Author

xjing76 commented Oct 13, 2021

@brandonwillard I just added the test cases for pm.Deterministic But when I am testing for that case it seems like within the step function the point (samples for beta)would be of wrong size. Could you take a look at this. I couldn't figure out why

I think what is happening is that when we compile the function for mu the sample results we are getting is actually (M, M) instead of (M, 1)

@xjing76 xjing76 force-pushed the gibbshs branch 3 times, most recently from 2478e41 to 53a26de Compare October 14, 2021 14:53
@xjing76 xjing76 force-pushed the gibbshs branch 3 times, most recently from 07eb1a2 to 98268aa Compare October 14, 2021 18:13
Copy link
Contributor

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

A few updates are needed, but this looks really good!

pymc3_hmm/distributions.py Outdated Show resolved Hide resolved
tests/test_step_methods.py Show resolved Hide resolved
tests/test_step_methods.py Show resolved Hide resolved
tests/test_step_methods.py Outdated Show resolved Hide resolved
pymc3_hmm/distributions.py Outdated Show resolved Hide resolved
Copy link
Contributor

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

The updates look good, but there's an error in the tests.

@xjing76 xjing76 force-pushed the gibbshs branch 4 times, most recently from 3cdc4fc to d7796f1 Compare October 25, 2021 13:30
@xjing76
Copy link
Contributor Author

xjing76 commented Oct 25, 2021

@brandonwillard Do you mind to take a look at line 554 that wasn't passing the coverage test ? When i put in the breakpoint() right before it I am able to get a break when I testing locally. However, for coverage test is just not passing.

@brandonwillard
Copy link
Contributor

@brandonwillard Do you mind to take a look at line 554 that wasn't passing the coverage test ? When i put in the breakpoint() right before it I am able to get a break when I testing locally. However, for coverage test is just not passing.

There's nothing that stands out from inspection alone. You'll need to look into the testing workflow and/or coverage settings to see if that's where the issue is.

Try adding a commit that outputs a debug print statement and see if it appears in the CI output (and exclusively runs the relevant test). You can remove the debug commit afterward.

@xjing76
Copy link
Contributor Author

xjing76 commented Oct 28, 2021

I think I am able to create a minimal example
with foo.py

def foo():
    for i in range(10):
        if i < 10:
            pass
        else :
            continue
    return 5

def foo2():
    for i in range(10):
        if i < 9:
            pass
        else:
            continue
    return 5

test_foo.py

from poj.foo import foo,  foo2
def test_foo():
    assert foo() == 5
    assert foo2() == 5

Both of the continue is reported not covered.

I reported this issue here

@jeffreyenos
Copy link
Contributor

It looks like this coverage issue is documented in nedbat/coveragepy#198 and is fixed with Python 3.10.

@brandonwillard
Copy link
Contributor

It looks like this coverage issue is documented in nedbat/coveragepy#198 and is fixed with Python 3.10.

Then we can just mark it as no cover (or whatever serves that purpose).

Copy link
Contributor

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

If you remove 6ad3330, squash the rest, and rebase onto main we can consider merging this.

@brandonwillard brandonwillard merged commit edf5c2e into AmpersandTV:main Nov 8, 2021
@brandonwillard brandonwillard changed the title horseshoe gibbs and tests Add a Horseshoe prior Gibbs sampler Dec 8, 2021
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