-
Notifications
You must be signed in to change notification settings - Fork 48
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 ability to display p/t-value below coefficient #69
base: master
Are you sure you want to change the base?
Conversation
@@ -111,7 +112,8 @@ def extract_data(self): | |||
covs = [] | |||
for md in self.model_data: | |||
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
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).
@@ -704,7 +716,7 @@ def generate_cov_precision(self, cov_name): | |||
cov_text += self._float_format(md['conf_int_low_values'][cov_name]) + ' , ' | |||
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: