fix: EvaluationResult serialization changes dataframes
#4906
Conversation
There was a problem hiding this comment.
The changes look good to me and they are ready to be merged. 👍 The changed handling of np.nan is not covered by any test though I think. The PR could be improved by adding a test case that loads an evaluation result from disk that contains np.nan in one of the dataframes. The test could confirm that the loaded dataframe contains not np.nan but None.
| @@ -1620,6 +1620,7 @@ def safe_literal_eval(x: str) -> Any: | |||
| node_results = {file.stem: pd.read_csv(file, **read_csv_kwargs) for file in csv_files} | |||
| # backward compatibility mappings | |||
| for df in node_results.values(): | |||
| df.replace(to_replace=np.nan, value=None, inplace=True) | |||
There was a problem hiding this comment.
The effect of this line is not tested, is it?
There was a problem hiding this comment.
Actually it is. The test of the generative pipeline produces None values for the context column. When reading the csv, we get np.nan values for this column. But I agree, it's not obvious. I'll write a test that makes this behavior explicit.
Related Issues
EvalResultare not the same after (de)serializing #4905Proposed Changes:
How did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.