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

Fix spelling/grammar in pipeline notebook #1082

Merged

Conversation

abigsmall
Copy link
Contributor

@abigsmall abigsmall commented Jul 7, 2024

Noticed a few misspellings. I hope I’m not taking up valuable time by submitting this PR :).

There were a couple places I wasn’t sure what the intended meaning was, so I did not try to fix those, but would like to call them out below:
under Prediction:

During interference, the augmenter does only extract the relevant features it has found out in the training phase and the classifier predicts the target using these features.

  • Was “interference” intended to be used here? I’m new to tsfresh and to machine learning in general, but I was thinking this sounded off.
  • To my ears the “does only” is a little awkward, and the rest of the sentence with “and the classifier” sounds like a run-off sentence.

under Separating the time series data containers:

During training, only the data with the ids from X_train were extracted and during prediction the rest.

  • I have attempted a rewrite but am unsure if it captures the intended meaning of the original:

During training, only the data with the ids from X_train were extracted. The rest of the data are extracted during prediction.

Copy link
Collaborator

@nils-braun nils-braun left a comment

Choose a reason for hiding this comment

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

Great! Thanks @abigsmall - much appreciated.
Can you apply the missing style fix (e.g. by running pre-commit?)

Regarding your the first sentence:
interference should actually be inference (ML inference).
Maybe we can change it to:

During interference, the augmenter only extracts those features that it has found as being relevant in the training phase. The classifier predicts the target using these features.

And regarding the second sentence: I like your proposal! Feel free to put it into the PR as well.

@abigsmall abigsmall force-pushed the fix-notebook02-sklearn-pipeline-grammar branch from 115cb15 to e2e22cd Compare July 17, 2024 00:06
Noticed a few misspellings. Fixed a couple more based on PR feedback.
@abigsmall abigsmall force-pushed the fix-notebook02-sklearn-pipeline-grammar branch from e2e22cd to 3647821 Compare July 17, 2024 00:13
@abigsmall
Copy link
Contributor Author

Hi @nils-braun, thank you for the feedback! I had initially just made my changes via the GitHub web UI and wasn’t sure how to run pre-commit manually so:

  1. cloned the repo locally
  2. set upstream to https://github.com/blue-yonder/tsfresh.git
  3. fetched/pulled latest upstream/main
  4. ran pip install -e ".[testing]"
  5. ran pre-commit install
  6. ran pytest
  7. made the additional wording changes based on your suggestions and committed
    • which kicked off the pre-commit hook[1]
  8. squashed my commits and your merge commits onto the updated main
  9. forced pushed to my abigsmall:fix-notebook02-sklearn-pipeline-grammar branch
    • i just prefer the cleanliness of squashing+rebasing but understand that merging is simpler/less prone to catastrophic errors :-)

Let me know if I had to do something else!

[1] terminal output after committing
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://github.com/pycqa/isort.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for https://github.com/psf/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pycqa/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
black................................................(no files to check)Skipped
isort................................................(no files to check)Skipped
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed

Copy link
Collaborator

@nils-braun nils-braun left a comment

Choose a reason for hiding this comment

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

LGTM!

@nils-braun nils-braun enabled auto-merge (squash) July 17, 2024 06:44
@nils-braun nils-braun merged commit 659f63d into blue-yonder:main Jul 17, 2024
4 checks passed
@nils-braun
Copy link
Collaborator

Thanks @abigsmall!
One small note: many repos on GitHub nowadays squash on PR merge, so you do not need to squash yourself (only really relevant for larger changes where it might be useful to see the individual commits - here it is totally fine)

@abigsmall abigsmall deleted the fix-notebook02-sklearn-pipeline-grammar branch July 17, 2024 17:38
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.

2 participants