Closed jzhang18 closed 6 years ago
Seems good to me. Just to be sure, Do you need this to avoid dropping data when you have that some forward return columns contain valid data but few of them have nan?
@luca-s yes, that's the reason.
Ok then but I would also add another dropna that discards all the rows for which we have nan in all the forward return columns. This would make clear the user didn't pass data for a particular time period. What do you think about this?
@jzhang18 there is also one last thing that makes me nervous about this change (which I like by the way). Since the first release of Alphalens we have had the line merged_data = merged_data.dropna()
so the code never had to deal with nans. What kind of tests have you done to verify there are no side effects after this change?
After talking about it with @jzhang18 we don't think this should be fixed in alphalens.
Modified the way to drop nan values for
merged_data
. By this way, it only drops the data that does not have validfactor value
orfactor_quantile
.