Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Disregard NaNs in preprocessing #177

Merged
merged 3 commits into from Jan 9, 2020

Conversation

eliwoods
Copy link

@eliwoods eliwoods commented Jan 7, 2020

This is a redo of #175; rebasing to the dev branch got hairy so I'm just making a new PR along with the suggested additions from #175 (comment)


If a time series with any NaN values is passed to TimeSeriesScalerMeanVariance.fit_transform() or TimeSeriesScalerMinMax.fit_transform() the transformed time series returns as all NaN

E.g:

In [1]: from tslearn.preprocessing import TimeSeriesScalerMeanVariance                                                                

In [2]: from numpy import nan                                                                                                         

In [3]: TimeSeriesScalerMeanVariance().fit_transform([0, nan, 6])                                                                     
Out[3]: 
array([[[nan],
        [nan],
        [nan]]])

This could be fixed by using the NaN disregarding numpy equivalent functions in the respective transform methods. This also makes tslearn in line with sklearn in terms of NaN's and preprocessing: scikit-learn/scikit-learn#10404

@pep8speaks
Copy link

pep8speaks commented Jan 7, 2020

Hello @eliwoods! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-08 21:50:30 UTC

@eliwoods
Copy link
Author

eliwoods commented Jan 8, 2020

@rtavenar addressed your additions. Should I make the update to CHANGELOG.md in this branch as well? I don't see anything in the contribution guidelines about the correct order.

@eliwoods eliwoods changed the title Disregard NaNs in preprocessing [MRG] Disregard NaNs in preprocessing Jan 8, 2020
@rtavenar
Copy link
Member

rtavenar commented Jan 8, 2020

@eliwoods yes, please document your change in CHANGELOG.md and I will merge your PR asap. Thanks for your contribution, it will be integrated in tslearn from version 0.3!

@eliwoods
Copy link
Author

eliwoods commented Jan 8, 2020

@rtavenar change log updated; feel free to merge. Cheers!

@rtavenar rtavenar merged commit 7cf8256 into tslearn-team:dev Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants