Skip to content

P outlier defaults#682

Merged
AlexanderFengler merged 11 commits intomainfrom
p-outlier-defaults
Mar 10, 2025
Merged

P outlier defaults#682
AlexanderFengler merged 11 commits intomainfrom
p-outlier-defaults

Conversation

@AlexanderFengler
Copy link
Copy Markdown
Member

adding in missing defaults for p-outlier regression, and a small associated tutorial.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/hssm/hssm.py 50.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
src/hssm/prior.py 89.18% <ø> (ø)
src/hssm/hssm.py 77.88% <50.00%> (ø)
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

lgtm

src/hssm/hssm.py Outdated
Comment on lines +1741 to +1745
# # Need to determine what the output should look like
# if self.p_outlier.is_regression:
# raise NotImplementedError(
# "Regression for `p_outlier` is not implemented yet."
# )
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Keep commented code or ok to delete?

Comment on lines +330 to +335
"z": {"dist": "Weibull", "alpha": 1.2, "beta": 0.25},
"t": {"dist": "HalfNormal", "sigma": 0.2},
"sv": {"dist": "Weibull", "alpha": 1.5, "beta": "0.3"},
"sz": {"dist": "Weibull", "alpha": 1.5, "beta": "0.3"},
"st": {"dist": "Weibull", "alpha": 1.5, "beta": "0.3"},
"sv": {"dist": "Weibull", "alpha": 1.5, "beta": 0.3},
"sz": {"dist": "Weibull", "alpha": 1.5, "beta": 0.3},
"st": {"dist": "Weibull", "alpha": 1.5, "beta": 0.3},
"p_outlier": {"dist": "Weibull", "alpha": 1.5, "beta": 0.3},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why the switch to actual number types?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To be honest I am confused how this became a string before. As far as I am concerned this should have always been float. @cpaniaguam @digicosmos86

Copy link
Copy Markdown
Collaborator

@digicosmos86 digicosmos86 left a comment

Choose a reason for hiding this comment

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

LGTM! I think we also need to change some associated type annotations and for p_outliers in HSSM and other functions to make explicit that we do support regressions for it. It could be another PR though

Copy link
Copy Markdown
Collaborator

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Still good. Thanks!

@AlexanderFengler AlexanderFengler merged commit a3b20a3 into main Mar 10, 2025
4 of 5 checks passed
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