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 negative binomial distribution #356

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

Conversation

ashler-herrick
Copy link

Added the negative binomial distribution by following the guide here: https://stanfordmlgroup.github.io/ngboost/5-dev.html

@ashler-herrick
Copy link
Author

@alejandroschuler Any tips on how to fix the github actions failing?

@alejandroschuler
Copy link
Collaborator

@ryan-wolbeck can probably help!

@ryan-wolbeck
Copy link
Collaborator

@ashler-herrick you will need to install Make in your development environment then run 'make lint' that will fix the formatting error you are seeing.

@ryan-wolbeck ryan-wolbeck self-requested a review September 9, 2024 01:25
@ashler-herrick
Copy link
Author

@ryan-wolbeck Any ideas on how to fix the failing tests? I've fixed the formatting errors, ran 'make test' locally (passed) but it seems to be failing for Python 3.9 through GH Actions

@ryan-wolbeck
Copy link
Collaborator

ryan-wolbeck commented Oct 4, 2024

@ashler-herrick it's probably failing only in the github action tests because your local environment is probably not 3.9. I would try creating a new conda env and setting it to 3.9 then do 'make' to install and re-run the tests. I'll pull the code here and take a look and get back to you with what I see.

@ryan-wolbeck
Copy link
Collaborator

ryan-wolbeck commented Oct 4, 2024

Interesting, when I run on 3.9 locally don't get the failure, this will take me more time to dig into. Going to also re-run the github actions to see if it's repeating

@ryan-wolbeck
Copy link
Collaborator

@ashler-herrick now when I re-run the actions I get the following linting error

************* Module ngboost.api
ngboost/api.py:57:4: R0917: Too many positional arguments (15/5) (too-many-positional-arguments)
ngboost/api.py:143:4: R0917: Too many positional arguments (13/5) (too-many-positional-arguments)
ngboost/api.py:235:4: R0917: Too many positional arguments (13/5) (too-many-positional-arguments)
ngboost/api.py:293:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments)


Your code has been rated at 9.86/10

************* Module ngboost.ngboost
ngboost/ngboost.py:53:4: R0917: Too many positional arguments (15/5) (too-many-positional-arguments)
ngboost/ngboost.py:175:4: R0917: Too many positional arguments (6/5) (too-many-positional-arguments)
ngboost/ngboost.py:204:4: R0917: Too many positional arguments (10/5) (too-many-positional-arguments)
ngboost/ngboost.py:262:4: R0917: Too many positional arguments (10/5) (too-many-positional-arguments)

@ryan-wolbeck
Copy link
Collaborator

@ashler-herrick

I've spent some time looking into this PR again, could you move the Negative Binomial tests into test_distns.py instead of test_score.py? Since Negative Binomial is a distribution, we keep all distribution tests together for consistency (e.g., like Normal, Laplace, etc.). Once moved, it’ll fit neatly with the rest of the distribution-specific tests.

If you can push a new commit for that I'll take another look to ensure linting issues are not occurring still.

Evn py 3.9
Run
make lint
make test

@ashler-herrick
Copy link
Author

ashler-herrick commented Dec 31, 2024

@ryan-wolbeck Will do, I appreciate the help. I'm a bit of a noob when it comes to the formalisms of software.

I also noticed some issues with my fisher information calculation so I'm going to revise that and do some additional testing.

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