Fix bug with null replication metrics when row is all null #706
Conversation
tonywu315
requested review from
JGSweets,
ksneab7,
taylorfturner,
micdavis and
tyfarnan
as code owners
taylorfturner
added
Bug
| data.loc[max_null_index] = {} | ||
|
|
||
| # Fill in missing rows with NaN and convert types to numeric | ||
| data = data.reindex(range(data.index.max() + 1), fill_value=np.nan).apply( |
There was a problem hiding this comment.
this could be really expensive for large data. Is there another way around this?
There was a problem hiding this comment.
What specific error is requiring this change / where?
There was a problem hiding this comment.
+1 good catch @JGSweets
There was a problem hiding this comment.
If a row is all null, the dataframe will be missing a row, which causes an error on the line
sum_null = data.iloc[null_indices, data.columns != col_id].sum().to_numpy().
reindex fills in these missing rows with nan.
There was a problem hiding this comment.
The .reindex() though when done on an entire dataset could be very costly in terms of runtime; so that is the concern around this operation that is being added to the process.
There was a problem hiding this comment.
null_indices + non-complete null rows?
There was a problem hiding this comment.
or even better potentially would be null_indices - complete null rows (theoretically there should be less completely null rows)
There was a problem hiding this comment.
Another way would be to reindex a single column in clean_samples and then combine all of them into data, which should also fix the bug.
There was a problem hiding this comment.
That would work too
There was a problem hiding this comment.
Any way we can do it w/o re-indexing? I think that would be the ideal state.
|
|
||
| # If the last row is all null, then add rows to the data DataFrame | ||
| max_null_index = max( | ||
| [max(i) for i in getattr(self._profile[0], "null_types_index").values()], |
There was a problem hiding this comment.
self._profile[0] Right now this is getting the first profile every time is this correct?
Why do we need to add rows to the df?
There was a problem hiding this comment.
If the last row(s) is all null, then the maximum null index of any profile should be the index of the last row.
There was a problem hiding this comment.
- in this test case,
len(self._profile)is3and only the zero-th index has values.
getattr(self._profile[0], "null_types_index").values()
dict_values([{2, 4}])
getattr(self._profile[1], "null_types_index").values()
dict_values([])
getattr(self._profile[2], "null_types_index").values()
dict_values([])
@tonywu315 is this always the case though? are we sure we can always just pull the zero-th profile in the self._profile list?
Head branch was pushed to by a user without write access
| sum_null = ( | ||
| data.loc[data.index.intersection(null_indices), data.columns != col_id] | ||
| .sum() | ||
| .to_numpy() | ||
| ) |
There was a problem hiding this comment.
This code does not error anymore if the entire DataFrame is null.
There was a problem hiding this comment.
this is much cleaner!
When
null_replication_metricsis enabled and any row is all null, this errors: