-
Notifications
You must be signed in to change notification settings - Fork 53
Add ability to display p/t-value below coefficient #69
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
base: master
Are you sure you want to change the base?
Conversation
| covs = covs + list(md['cov_names']) | ||
| self.cov_names = sorted(set(covs)) | ||
| # deduplicate cov_names list from multiple models with same dep-vars (but keep order seen) | ||
| self.cov_names = list(dict.fromkeys(covs)) |
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 you clarify what is the problem?
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.
Including this was oversight from me, it's a local change on my part that I didn't mean to include it in the t/p stat PR. This code does the same thing as sorted(set()) (removing duplicates) but doesn't sort the covariates, so the output order will be the same as the input order. I'll remove this change.
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.
Thanks
toobaz
left a comment
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.
Thanks for your PR: I like the general idea, happy to merge if you can better harmonize with existing code (see comments).
| cov_text += self._float_format(md['conf_int_high_values'][cov_name]) | ||
| else: | ||
| cov_text += self._float_format(md['cov_std_err'][cov_name]) | ||
| cov_text += self._float_format(md[self.sig_stat][cov_name]) |
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 understand that this approach is very clean, but given that in the future we would ideally allow to combine confidence intervals, standard errors and p-values, please add a new "else" clause for the latter.
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.
As in elif self.p_values (where, say, self.p_values is a flag like self.confidence_intervals)? I guess I don't understand what you mean, is there code for combining these three I can pattern off of?
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.
elif self.sig_stat == ... is also OK, as you prefer.
|
|
||
| def show_tvalues(self): | ||
| """Show T-statistics in parentheses below coefficients""" | ||
| self.sig_stat = 'cov_tvalues' |
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.
This method makes sense... as long as it also reverts back show_confidence_intervals
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.
As in add self.show_convidence_intervals(False)? That makes sense, seems obvious in hindsight.
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 ideal behavior would be that self.show_*(True) shows only what is being asked, while self.show_*(False) removes what is being asked if it is currently shown, and has no effect otherwise. I think what you're suggesting works.
This small changes adds an easy way of changing SE below coefficients in parentheses to p-value or t-stat. The change is pretty minor, adding two convenience functions: