feat: adding TwoEventsWashover base class and was modified validate c… #167
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
- Coverage 97.00% 94.71% -2.29%
==========================================
Files 9 9
Lines 902 928 +26
==========================================
+ Hits 875 879 +4
- Misses 27 49 +22 ☔ View full report in Codecov by Sentry. |
| f"{end_time_column = } is not in the record dataframe columns and/or not specified as an input." | ||
| ) | ||
|
|
||
| def washover( |
There was a problem hiding this comment.
I'd try to maintain the signature of the abstract washover class, if not it's very hard to put it into power analysis:
@abstractmethod
def washover(
self,
df: pd.DataFrame,
truncated_time_col: str,
treatment_col: str,
cluster_cols: List[str],
original_time_col: Optional[str] = None,
) -> pd.DataFrame:
"""Abstract method to add washvover to the dataframe."""There was a problem hiding this comment.
For this reason, I'd send start and end columns to the init method
|
|
||
| def __init__( | ||
| self, | ||
| calendar_df: pd.DataFrame, |
There was a problem hiding this comment.
I'd try to avoid do to init with a calendar df, we've tried to keep constructors only based on very simple types like str, int and float. I'd assume the user already has the calendar in the record df, so you don't need to do a join, and it works like other washover classes
There was a problem hiding this comment.
okis, fixing in the next commit.
base class and was modified validate columns