-
Notifications
You must be signed in to change notification settings - Fork 12
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
First attempt at Quantile probability plot #319
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.
Looks great.
Left a few comments that should be very simple to accomodate.
Otherwise, let's do final debugging simply by battle-testing :)
).reshape(out_shape) | ||
lapse_response = rng.binomial(n=1, p=0.5, size=out_shape) | ||
).reshape(replace_shape) | ||
lapse_response = rng.binomial(n=1, p=0.5, size=replace_shape) |
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.
here I would go with np.random.choice(n_choices, ...) to accomodate models that have more than 2 choices.
if n_choices == 2, we change 0 --> -1, otherwise keep choices as they are.
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.
Can we tab this for now since it might take a bit more engineering to figure out where that n_choices
comes from. ssms
would need that to figure out how many responses to generate right?
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.
yeah. That should be part of model configs actually so should be easily accessible. (can double check but I think it is for ssm-simulators?)
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.
Also outputs from ssms.simulator() have the 'metadata' key, which also has that.
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.
I mean we do have one step to figure out number of responses from the data, but we might want the user to be a bit more explicit in non-binary cases. In any case we might need a separate PR to address this
_logger = logging.getLogger("hssm") | ||
|
||
|
||
def _plot_quantile_probability_1D( |
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.
some of these plotting parameters might be passed through to the plot as **kwargs?
(I know I didn't do this well in HDDM :))
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.
reading a bit more I think you already tried to account for that quite a bit, but took the choice e.g. to keep xlabel and ylabel as named arguments here?
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.
Sorry just saw this. Yeah I am doing xlabel
and ylabel
from experience with R lol. It's hard to decide which ones you explicitly declare and which ones can be included in kwargs
. I just thought that the users might want to be able to change these labels quickly
|
||
# get the posterior samples | ||
idata_posterior = idata["posterior_predictive"]["rt,response"] |
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.
seems like logic wise, you could have idata is None --> True
but then still access idata
in this line?
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 function returns if idata is None
at line 121, so it would not reach here if idata is None
?
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.
ok overlooked the return
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.
ok looks good now :)
|
||
# get the posterior samples | ||
idata_posterior = idata["posterior_predictive"]["rt,response"] |
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.
ok overlooked the return
I have done the following in this PR:
seaborn
to basicmatplotlib
plots with the ability to plot regions of uncertainty