[Issue-400] Implementing dynamic alpha for coefplot and iplot #528
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Hi @s3alfisc , this will be the merge request for implementing dynamic alpha for iplot and coefplot I will re-do a sanity check, remove unnecessary files (used for testing), and put the proof of the test later Thanks |
| _coefnames = self._coefnames | ||
| if alpha is None: | ||
| ub, lb = 0.975, 0.025 | ||
| self.get_inference() |
There was a problem hiding this comment.
I recall the get_inference, as if the user had inputted non-default alpha for tidy previously, then the user recall tidy again, the value of confidence interval will be the non-default eventho the dataframe columns will show a default confidence interval
What do you think? @s3alfisc
|
Hi @s3alfisc , kindly need your review for this merge request, I've also included the test results. Let me know if there are any improvements or changes needed. Thanks! Test proofs:
|
add doc string for `alpha` argument to `tidy()`
unify alpha default for `get_inference`
|
Hi @rafimikail , I made minor adjustments. Will merge this after the CI tests pass =) Congrats 🎉 and thanks for your first contribution to pyfixest! |
|
@all-contributors please add @rafimikail for code |
|
I've put up a pull request to add @rafimikail! 🎉 |
| @@ -1490,7 +1490,7 @@ def get_performance(self) -> None: | |||
| self._adj_r2 = np.nan | |||
| self._adj_r2_within = np.nan | |||
|
|
|||
| def tidy(self, alpha=0.05) -> pd.DataFrame: | |||
| def tidy(self, alpha: Optional[float] = None) -> pd.DataFrame: | |||
| """ | |||
| Tidy model outputs. | |||
|
|
|||
There was a problem hiding this comment.
Could we add a Parameters section to the docstring here that explains the alpha argument and its default?
| self.get_inference() | ||
| else: | ||
| ub, lb = 1 - alpha / 2, alpha / 2 | ||
| self.get_inference(alpha=1 - alpha) |
There was a problem hiding this comment.
Here we are inconsistent with the naming of the function arguments.
For tidy, alpha correctly describes the "size" of the test, while for .get_inference, it is 1-alpha.
Could you change
def get_inference(self, alpha: float = 0.95) -> None:to
def get_inference(self, alpha: float = 0.05) -> None:and within get_inference, change
z = np.abs(t.ppf((1 - alpha) / 2, df))
# to
z = np.abs(t.ppf((alpha) / 2, df))and
z = np.abs(norm.ppf((1 - alpha) / 2))
# to
z = np.abs(norm.ppf(alpha / 2))?
Codecov ReportAttention: Patch coverage is
|
|
Hi @s3alfisc , I appreciate your help with the alpha consistency in get_inference, I was actually aware of it but I was concerned it might have dependencies with other functions, so i did not change it directly. Sorry could not address it myself immediately, it was quite late here in my country when you commented. Also Big Thanks! I'm really pleased with my first contribution and I am interested to keep contributing! |
No description provided.