Open goldingn opened 6 years ago
I'm not convinced we want to automatically na.omit()
based on covariates either.
The two options that seem reasonable to me are
RemoveNAs
process module (which I've written and just waiting to find time to upload).na.rm
argument to workflow
.Personally I like the first.
I agree 1 is the ideal solution. This would also require a review of existing models to ensure they work with NA
values and addition of NA
values to the module checking routines
But the current models don't need to work with NA values. I guess it would be good if they could error in a useful way if they can't handle NAs. Maybe that's what you mean by "work with NA values". But the workflow I imagine is:
Yes, I mean throw a sensible error or remove NAs with warning
OK yes then I totally agree. Should throw a sensible error.
Yeah, I agree. Best thing is to make the existing models remove NAs (with a warning) so we don't break any existing workflows,and to remove na.omit here.
Just to go back to this the RemoveNAs
module is now in the repo. I don't think Background
has had it's na.omit line removed.
Background
has anna.omit()
step before returning the dataframe, which removes rows if any column has NA values.Ideally, this should only remove rows with NAs in the the covariate columns (those columns named in the
covCols
attribute), since otherwise it causes problems if there are user-defined columns (#414).