-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added UserParam class for user parameter specification #579
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prob warrants a comprehensive tutorial on prior setting at this point?
Looks good otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I pointed out a couple of things I would change but might not be needed perhaps due to conventions that I'm not aware of yet.
self.args: dict = {} | ||
|
||
def to_dict(self) -> dict[str, Any]: | ||
"""Convert the object to JSON. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type of the return object is a dict
(also suggested by the function's name). Should the docstring be in line with this?
"""Convert the object to JSON. | |
"""Convert the object to a dictionary. |
return result | ||
|
||
@staticmethod | ||
def from_dict(d: dict) -> "Prior": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def from_dict(d: dict) -> "Prior": | |
def from_dict(d: dict) -> Prior: |
@staticmethod | ||
def from_bambi( | ||
bambi_prior: bmb.Prior, bounds: tuple[float, float] | None = None | ||
) -> "Prior": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) -> "Prior": | |
) -> Prior: |
tests/test_prior.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the truthy/falsy nature of the values in many of these checks? Makes for more consice code.
@digicosmos86 is this on hold or pushed through #590 at this point? |
From what I can see this is redundant now. |
Note: To avoid conflict with
src/jhssm/param.py
, I am naming the directorysrc/hssm/param_refactor
for now. Once all integration is tested, I will rename this directoryWhat was done in this PR:
prior.py
to prepare for model outputhssm.Param
) for users to specify priors and store user specification