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

[DOC] added post-processing notes in tutorial #3878

Merged

Conversation

nshahpazov
Copy link
Contributor

@nshahpazov nshahpazov commented Dec 3, 2022

Reference Issues/PRs

Description of PR

  • Checked and confirmed that dunder pipeline methods are described in the tutorial
  • Added myself to the list of contributors.
  • Added description in the tutorial on how TransformedTargetForecaster can also end with a post-processing Transformer, in this case, I've used a simple predictions rounding

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nshahpazov nshahpazov changed the title [DOC] added post-processing notes in tutorial #2608 [DOC] added post-processing notes in tutorial Dec 3, 2022
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

That´s nice, thanks!

May I ask for one thing, to keep the repository size small: can you make sure only the parts are changed where you add content?

I think the current huge diff comes from you rerendering plots.

Worst case, this might require going into the ipynb file with an editor, but should also work if you take the original file and copy-paste the change.

@nshahpazov
Copy link
Contributor Author

@fkiraly I've addressed the proposed changes. The actual diff is visible now.

@aiwalter
Copy link
Contributor

aiwalter commented Dec 4, 2022

also please make sure you run pre-commit bcz linting is failing

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks! Great idea to add this content, will be useful to our users!

Change requests:

  • linting
  • I would split the content and introduce it step by step, or it may be confusing if it comes all at once, see suggestion above

@nshahpazov
Copy link
Contributor Author

@fkiraly I've rephrased the paragraphs into three sections like you suggested. Linting is working now. Let me know if you find it ok.

@nshahpazov nshahpazov requested review from fkiraly and removed request for GuzalBulatova December 9, 2022 08:13
fkiraly
fkiraly previously approved these changes Dec 19, 2022
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

All good! Just made some minor changes in formatting.

@lmmentel lmmentel merged commit fae39a8 into sktime:main Dec 20, 2022
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

4 participants