fixed issue with run live or frozen #119
Conversation
| 5,C,5,5 | ||
| 6,E,6, | ||
| 7,W,7, | ||
| target,error,live,frozen,frozen_error |
There was a problem hiding this comment.
Why does frozen_error have a numerical type? Does this not relate to the other error in col2?
There was a problem hiding this comment.
Frozen_error contains the deleted adjusted values.
| @@ -316,8 +317,10 @@ def run_live_or_frozen( | |||
| ) | |||
|
|
|||
| if state == "frozen": | |||
|
|
|||
| df.loc[df[error_marker].isin(error_values), target] = np.nan | |||
| df["frozen_error"] = df.apply( | |||
There was a problem hiding this comment.
Nice use of lambda function to simplify copying over the value from the target column :)
| @@ -104,13 +104,16 @@ def test_run_live_or_frozen(filepath): | |||
|
|
|||
| df = pd.read_csv(filepath / "test_run_live_or_frozen.csv") | |||
|
|
|||
| df_in = df.drop(columns=["frozen"]) | |||
| df_in = df.drop(columns=["frozen", "frozen_error"]) | |||
There was a problem hiding this comment.
Guessing you are using one input data to have the input and correct output which is why its being dropper early in test?
| expected_output_frozen = df.copy() | ||
|
|
||
| expected_output_frozen.drop(columns=["frozen"], inplace=True) | ||
| expected_output_frozen = expected_output_frozen.fillna("") | ||
|
|
||
| assert_frame_equal(frozen_output, expected_output_frozen) | ||
| assert_frame_equal(live_ouput, df_in) |
There was a problem hiding this comment.
Never noticed this before, but what happens if the first assert fails, but the second passes? or vice versa? Just want to make sure the unit test fails if both or one fail
There was a problem hiding this comment.
Checked together and it fails if either one fails.
There was a problem hiding this comment.
Sorted this, agreed that if one fails the unit test fails. Only appears as one unit test though
Summary
Add your summary here - keep it brief, to the point, and in plain English.
Checklists
This pull request meets the following requirements:
If you feel some of these conditions do not apply for this pull request, please
add a comment to explain why.