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

[BUG] changed wrong docstring default value of start_with_window in SlidingWindowSplitter to actual default value #3340

Merged
merged 10 commits into from Aug 27, 2022

Conversation

pmuls99
Copy link
Contributor

@pmuls99 pmuls99 commented Aug 25, 2022

Reference Issues/PRs

Fixes #3315.

What does this implement/fix? Explain your changes.

The default value was mentioned to be False, but it should be True, in the code, it has been rightly used but, in the comments, there was an error. Changed the default value.

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

@pmuls99 pmuls99 changed the title Changed default value in start_with_window [BUG] Changed default value in start_with_window Aug 25, 2022
@fkiraly fkiraly changed the title [BUG] Changed default value in start_with_window [BUG] changed wrong docstring default value of start_with_window in SlidingWindowSplitter to actual default value Aug 25, 2022
fkiraly
fkiraly previously approved these changes Aug 25, 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.

Straightforward fix, thanks!

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 25, 2022

the bug has been slayed, but the linter is dismayed

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 25, 2022

... or, slain?

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 25, 2022

I am not sure why are these tests are failing :(. Can you tell what can I do now?

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 25, 2022

code formatting. Click here to see the code quality checker report:
image

You can also set this up locally - very helpful, see sktime developer gude here:
https://www.sktime.org/en/stable/developer_guide/coding_standards.html

Side note: it is in code that you didn't change, but it rechecks entire files where changes occur. There was a change in the linter defaults around exponent spacing recently, so now it notices it for the file you changed.

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 26, 2022

@fkiraly do i need to create a new PR now ?

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 26, 2022

@fkiraly do i need to create a new PR now ?

No, just push new commits to your branch, and the PR will update, too.

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 26, 2022

@fkiraly, have done. Can you see once.

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 26, 2022

@aiwalter Can you take a look?

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 26, 2022

@fkiraly, @aiwalter code satisfying the coding standards. Still, the tests are failing. Does this mean that this code might not work on those Operating systems? If yes, what might be the fix, I have observed similar tests failing in other PRs as well.

@pmuls99 pmuls99 requested review from fkiraly and removed request for aiwalter August 26, 2022 18:31
@fkiraly
Copy link
Collaborator

fkiraly commented Aug 26, 2022

Failures are not your fault, related to another estimator that made it onto main by accident (the one failing the test), see here: #3350

if you update your branch from main, it should all be fine.

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 27, 2022

@fkiraly, can you approve the running workflow?

@pmuls99
Copy link
Contributor Author

pmuls99 commented Aug 27, 2022

@fkiraly all the tests have passed. Now can you please merge the code?

@fkiraly fkiraly merged commit 86506e1 into sktime:main Aug 27, 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.

[DOC] Docstring lists wrong default for SlidingWindowSplitter parameter start_with_window
2 participants