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

fixed missing datetime for timebased features and allowed warnings to be silenced #874

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Apsylem
Copy link

@Apsylem Apsylem commented Jul 1, 2021

dask_feature_extraction_on_chunk did not forwardthe sort_by column as index to the time dependend feastures such al 'linear_trend_timewise. Also The option to forbid warnings has been added 'dask_feature_extraction_on_chunk'

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2021

Codecov Report

Merging #874 (08b2ef0) into main (ff69073) will decrease coverage by 0.14%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #874      +/-   ##
==========================================
- Coverage   95.34%   95.19%   -0.15%     
==========================================
  Files          18       18              
  Lines        1867     1872       +5     
  Branches      368      369       +1     
==========================================
+ Hits         1780     1782       +2     
- Misses         47       49       +2     
- Partials       40       41       +1     
Impacted Files Coverage Δ
tsfresh/convenience/bindings.py 53.33% <55.55%> (-2.67%) ⬇️
tsfresh/feature_extraction/extraction.py 95.06% <100.00%> (ø)
tsfresh/feature_extraction/feature_calculators.py 97.60% <100.00%> (ø)

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 ff69073...08b2ef0. Read the comment docs.

@Apsylem
Copy link
Author

Apsylem commented Jul 2, 2021

fixed style issues

@nils-braun
Copy link
Collaborator

Hi @Apsylem ! Sorry the review takes so long, I am a bit occupied in the moment. I will try to have a look in the next few days!
Thank already for your changes!

@nils-braun
Copy link
Collaborator

Thanks @Apsylem - the changes look good to me. Could you include the current main branch? We have now added styling with black using a pre-commit hook.
I would propose to merge main into your branch, install the pre-commit script and run it once in your changes. Of you need some help, please get in touch with me :-)

@Apsylem
Copy link
Author

Apsylem commented Jul 9, 2021

Sry,
I already performed the tests locally, as described in how_to_contribute or did u mean something different by "pre-commit script" ?

@nils-braun
Copy link
Collaborator

nils-braun commented Jul 9, 2021

Yeah, we very very recently introduced a separate way for the styling. If you want, I can take care of it (it is described in the newest version of the page you referring to, if you have a look into it on the repo directly)

Update: sorry, my bad. I actually did not push the changes to the docs, so you couldn't know. Sorry about that!

@nils-braun
Copy link
Collaborator

nils-braun commented Jul 9, 2021

I have included the needed style changes to your branch :-) The information on how to run the pre-commit hooks (and the styling) is now also on our documentation page.

One thing I noticed while doing the style fix: in your last commits you also included one commit from your other PR (the one which adds the taste_of_df parameter and the meta interference).

While it is generally ok to have PRs which build on each other, please do not mix them and - if possible - please do never include multiple features in one PR. This makes the review very hard. In this case, please revert the changes for the taste_of_df and the meta data interference from this PR - we can discuss them separately. In principle, it would also be wise to have the additional show_warnings parameter as a separate PR, but we can keep it like it is for now.

If you need help in reverting the additional changes, please get in touch with me! Thanks.

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.

3 participants