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

better meta inferrence, to prevent indexing error while parquetting #877

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

Conversation

Apsylem
Copy link

@Apsylem Apsylem commented Jul 9, 2021

a unneccary indexing over cloumns in _feature_extraction_on_chunk_helper has been removed

but mainly, an option "taste_of_pandas_df" has been addet to dask_feature_extraction_on_chunk.
This example of the input is used to infer the correct meta in df.apply. This was neccecery due to missing and misleading index values while parquetting the data after tsfresh feature extraction. No Bug report has been filed for this issue.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2021

Codecov Report

Merging #877 (b81e6bc) into main (b9a7b18) will decrease coverage by 0.14%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #877      +/-   ##
==========================================
- 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 b9a7b18...b81e6bc. Read the comment docs.

@nils-braun
Copy link
Collaborator

This PR also includes the changes of #874. I will wait for #874 to be finished (minus the changes in this PR) - after that we can have a look into the changes here individually.

@Apsylem
Copy link
Author

Apsylem commented Jul 12, 2021

I am sorry, this is my first pull request in a row and one of my first in general. Is there a better way to build upon my own changes, if the old pull request has not yet been accepted? I would aapreciate any remarks.

@nils-braun
Copy link
Collaborator

Cool! Then thanks for taking your time and starting with open-source development!

This is a good question and there are some options:

  • Easiest solution is to just wait for the other PR to be included before creating a new one. That sounds trivial, but is the solution I normally use.
  • If you want to open the PR anyway, open it against the older PR. We can then work on both PR in parallel, the changes of the old PR are automatically included in the new one and are still separate. In this case, you typically merge the PRs in the order "new, old". Just make sure you mention all related persons and explain them why we have two PRs.
  • You can also open the PRs as you have done but clearly state the fact that those depend on each other. I would however not recommend to do so, because it is hard to review the changes independently.

The absolut best option is to separate the changes into two independent PRs. That is not always possible though.

@Apsylem
Copy link
Author

Apsylem commented Aug 24, 2021

I am sorry, but I don't see the whitespaces, the error report on my other pull request, showed the locations to be modified.

here it says :

  • hook id: trailing-whitespace
  • exit code: 1
  • files were modified by this hook

does this mean, the trailing whitespaces have already been removed ?

@nils-braun
Copy link
Collaborator

nils-braun commented Aug 24, 2021

Hi @Apsylem !
You can check the full log of the automatic build result (or by clicking on "Details"). It will show you all problematic lines. The build will not edit your code (fortunately ;-)) and will therefore not fix the style errors for you.

If you have followed our installation for developers description on the docu, you should have "pre-commit" installed, which will trigger fixing these style issues locally on your computer before you push to github. If you do not want to do this, you can at least call black and isort by yourself (but using pre-commit is preferred).
If you do not know pre-commit, have a look into its docu.

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