Implementing outcome plot for panelview function #581
Conversation
rafimikail
changed the title
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Hi @rafimikail , thanks a lot! I have added an example to the DiD notebook =) Looks already quite good! Next steps would be to add docstrings, tests, and make sure that the |
|
Hi @s3alfisc , i have finished the development, changes i made:
I would appreciate it if you could review this when you have a moment. Thank you! |
|
Awesome, thank you Rafi! Will review this tomorrow! |
|
Plots look fabulous at first sight! |
|
Hi @s3alfisc , i have added it to the docs as well as fixed the code so it passed the ruff when committing, kindly have a check again, Thanks! |
pyfixest/did/visualize.py
Outdated
| f.colorbar(cax) if legend else None | ||
| ax.set_xlabel(xlab) if xlab else None | ||
| ax.set_ylabel(ylab) if ylab else None | ||
| if type == "outcome" and outcome: |
There was a problem hiding this comment.
Can we raise an informative error message when a user specifies type = "outcome" but does not provide an outcome variable?
I.e. we could do
if type == "outcome" and not outcome:
raise ValueError("You specified ... but ...")We would then also add a test in test_error_warnings.py
pyfixest/did/visualize.py
Outdated
| ax.set_xlabel(xlab) if xlab else None | ||
| ax.set_ylabel(ylab) if ylab else None | ||
| if type == "outcome" and outcome: | ||
| if units_to_plot: |
There was a problem hiding this comment.
For the units_to_plot argument, I would say that we should not allow subsampling and aggregation, right? Because if a user specifies specific units, does it make sense to still aggregate them / to subsample from them? (I think you could maybe argue for the first?). Still I would rather restrict this behavior.
pyfixest/did/visualize.py
Outdated
| data_pivot = data_pivot.sample(subsamp) | ||
| if collapse_to_cohort: | ||
|
|
||
| def get_treatment_start(x: pd.DataFrame) -> pd.Timestamp: |
There was a problem hiding this comment.
Maybe we can be more explicit here? E.g. change to
treat_bool = x[treat]
return x.iloc[treat_bool, time].min()
|
Hi @rafimikail , lots of confusion about this one, but the main outcome is that it's merged! |
|
Thank you! |
Hi @s3alfisc , this will be the PR for Issue #582 to enhance panelview to be able to do outcome plot.
I still need to finalize this by doing sanity check on the results and might also need to add the run tests in
test_visualize.py, I will update you once its complete