Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] fix
temporal_train_test_split
for hierarchical and panel data in case wherefh
is not passed #5330[BUG] fix
temporal_train_test_split
for hierarchical and panel data in case wherefh
is not passed #5330Changes from 37 commits
097f5a6
75f8c77
b5c7912
69a93f5
234a361
5f47f51
504df4e
2c56c0f
02a642a
0425e15
153171a
d7285a3
c9fe290
4ec9a10
0357421
e536bff
c1ec099
24c2b89
fa4d02a
fb591ee
58c4f60
6ffe1b4
b8dda31
7cf6c54
0d11ad7
56be74b
1fd85b5
0986c71
c61ce21
641437c
fe47289
292f996
4ad2580
f4d32a5
5334209
6de3de0
bbee8a8
18cebc5
ce2592c
612e408
2418aac
f2b9571
a7a2250
4a8da32
3a97912
efa1dec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably does not need an else block, since if block contains a return already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the else block made the condition easier to spot for a reader of the code, because it matches the indentation and can be tracked up to the
if
condition.If we remove the
else
, theif
condition will be harder to find.This is my rationale (in general) to do this even if you have
if
-return
, but I'm not too strongly opined about this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance, I did the opposite with
if fh is not None:
, because theelse
was very long.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a change to the code flow, and added comments - better? I went with your suggestion, but added comments to the
fh
and theX
branching to make it clear to the readerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a non-blocking comment really. Usually tools like
pylint
will complain against these (ref. R1705).The change looks good to me.
pylint
or etc. may still complain that function return signature vary between different return statements (ref. R1710), but I don't think there's a way to solve that here at all.