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
[ci] [python] reduce unnecessary data loading in tests #3486
[ci] [python] reduce unnecessary data loading in tests #3486
Changes from all commits
79c7007
f792015
c71a30f
979b76a
a86a6c2
afff7d3
c3067ee
25a9bc7
3af0a5c
999c5b6
3394222
3a5fe60
c1cb1b2
16ec8aa
4c2e7be
dfb0fd3
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.
Please memoize this too
LightGBM/tests/python_package_test/test_plotting.py
Line 19 in 5cc9e67
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.
what would be the purpose? That's the only call to
load_breast_cancer()
in that module. So I think the caching would add a tiny bit of overhead for no benefit.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.
How does it differ with other calls you've memoized? 5 calls of
load_breast_cancer()
intest_plotting.py
is even more than 3 calls of the same function intest_basic.py
, for example.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.
there is only 1 call in
test_plotting.py
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.
Yes, you are right, but the method in which this call is performed (
setUp
) is called before each test. So, actually we have 5 calls.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.
OOOOOOOOOOO haha ok. I haven't used
unittest.TestCase
in a while, I forgot which one was a "run before every test" setup and which one was a "run exactly once, before any tests" one.Ok yes I'll update 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.
added in dfb0fd3
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.
Thanks! Actually I think we should refactor this to "run exactly once, before any tests" (
setUpClass()
), but it is another issue, of course.