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

Deprecate functions for specific versions of Dynamic Time Warping #63

Merged
merged 15 commits into from Feb 19, 2020

Conversation

johannfaouzi
Copy link
Owner

This PR deprecates the functions for specific versions of Dynamic Time Warping. Similarly to scipy.optimize.minimize, only the main function is in the public API, while all the functions for the specific methods are private.

Notable changes:

  • All dtw_ functions are deprecated in 0.11 and will be removed in 0.12.
  • The corresponding privates functions, _dtw_*, are kept and their docstrings are updated to describe only the method-specific parameters.
  • Each version has its own page in the documentation, thanks to a custom Sphinx directive that is heavily inspired from the scipy-optimize custom directive.
  • A new function show_options has been added to show the options for each method.

Some stuff in this PR is not related to this topic, just maintenance updates.

@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@b0907bc). Click here to learn what that means.
The diff coverage is 99.83%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #63   +/-   ##
=========================================
  Coverage          ?   98.36%           
=========================================
  Files             ?       91           
  Lines             ?     4826           
  Branches          ?        0           
=========================================
  Hits              ?     4747           
  Misses            ?       79           
  Partials          ?        0
Impacted Files Coverage Δ
pyts/preprocessing/transformer.py 96.42% <ø> (ø)
pyts/multivariate/utils/utils.py 100% <ø> (ø)
pyts/approximation/sax.py 100% <ø> (ø)
pyts/metrics/__init__.py 100% <ø> (ø)
pyts/utils/utils.py 100% <ø> (ø)
pyts/decomposition/ssa.py 100% <ø> (ø)
pyts/metrics/boss.py 100% <ø> (ø)
pyts/transformation/weasel.py 99.35% <100%> (ø)
pyts/image/mtf.py 100% <100%> (ø)
pyts/bag_of_words/tests/test_bow.py 100% <100%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0907bc...e21face. Read the comment docs.

@johannfaouzi
Copy link
Owner Author

@johannfaouzi
Copy link
Owner Author

@hichamjanati Feel free to add any comment regarding this PR (especially on the docstrings, if the descriptions are clear enough).

Copy link
Contributor

@hichamjanati hichamjanati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all dtw_method functions are deprecated but when a specific method arg is passed to dtw the private funcs _dtw_method are called ?

Is the plan is to remove those in the future as well and keep the minimal code:
-> compute region
-> compute accumulated cost

  • eventually the fast and multi-resolution loops in private funcs ?

pyts/metrics/dtw.py Outdated Show resolved Hide resolved
@johannfaouzi
Copy link
Owner Author

So all dtw_method functions are deprecated but when a specific method arg is passed to dtw the private funcs _dtw_method are called ?

Is the plan is to remove those in the future as well and keep the minimal code:
-> compute region
-> compute accumulated cost

  • eventually the fast and multi-resolution loops in private funcs ?

For the moment the private _dtw_* functions are indeed called. I don't mind having fewer functions to make it simpler, but the code is already here and is tested, and adding new methods could easily be done by creating a new private _dtw_* function that would make use of existing functions.

We also need a function for each method, just to have the corresponding docstring for the description of the options (although we could create a function with this docstring that would do nothing).

pyts/metrics/dtw.py Outdated Show resolved Hide resolved
pyts/metrics/dtw.py Outdated Show resolved Hide resolved
pyts/metrics/dtw.py Outdated Show resolved Hide resolved
pyts/metrics/dtw.py Outdated Show resolved Hide resolved
pyts/metrics/dtw.py Outdated Show resolved Hide resolved
pyts/metrics/dtw.py Outdated Show resolved Hide resolved
of 'multiscale': the time series are downsampled so that the new time
series are very small, the optimal path for these time series is computed
and is projected at the scale that is two times larger. This process is
repeated until the original scale of the time series is reached.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These descriptions are better than the ones provided in the individual docstrings. Maybe it's worth replacing them with these.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier because we can talk about all the methods and how they relate to each other, while the individual docstrings need to be independent.

It's quite similar to scipy.optimize.minimize: the Notes section is very detailed, while the docstring for each method (e.g. minimize(method='Nelder-Mead')) is very short and does not give much information.

@hichamjanati
Copy link
Contributor

Ah right, I forgot about the individual pages in the doc. Well the duplicated code is very small, I think it's not worth changing if we have to replace the func with empty ones.

@hichamjanati
Copy link
Contributor

Good for me :) !

@johannfaouzi johannfaouzi merged commit faf550b into master Feb 19, 2020
@johannfaouzi johannfaouzi deleted the fix-dtw-deprecation-issues branch February 19, 2020 18:57
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

2 participants