Skip to content

Conversation

@carmocca
Copy link
Contributor

What does this PR do?

  • Create HookedDataModule to append the called hooks dynamically. Any new hook will make the test fail. We want this so there's no forgetting to update the test.
  • The hook arguments are now tested
  • A small fix is included to only include the dataloader_idx argument during predict if there is more than one dataloader. This is consistent with the rest of the loops.

Related to the Lightning hooks review: #7740

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • [n/a] Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

@carmocca carmocca added bug Something isn't working ci Continuous Integration labels Jun 11, 2021
@carmocca carmocca added this to the v1.4 milestone Jun 11, 2021
@carmocca carmocca self-assigned this Jun 11, 2021
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #7941 (09c83eb) into master (20a5e09) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7941   +/-   ##
======================================
- Coverage      92%     91%   -0%     
======================================
  Files         203     203           
  Lines       13129   13125    -4     
======================================
- Hits        12017   11974   -43     
- Misses       1112    1151   +39     

@carmocca carmocca added the data handling Generic data-related topic label Jun 11, 2021
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGMT !

@carmocca carmocca added the ready PRs ready to be merged label Jun 12, 2021
@kaushikb11 kaushikb11 enabled auto-merge (squash) June 13, 2021 04:34
@mergify mergify bot removed the has conflicts label Jun 13, 2021
@kaushikb11 kaushikb11 merged commit 436fc53 into master Jun 14, 2021
@kaushikb11 kaushikb11 deleted the tests/improve-hook-test-datamodule branch June 14, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci Continuous Integration data handling Generic data-related topic ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants