Skip to content

Standardize positional datamodule and argument names#7431

Merged
awaelchli merged 27 commits intomasterfrom
feature/datamodule-as-arg
Jun 15, 2021
Merged

Standardize positional datamodule and argument names#7431
awaelchli merged 27 commits intomasterfrom
feature/datamodule-as-arg

Conversation

@carmocca
Copy link
Copy Markdown
Contributor

@carmocca carmocca commented May 7, 2021 •
edited
Loading

What does this PR do?

Fixes #7425

  1. DataLoader typing
  2. Consistent argument names
  3. Consistent support for DataModule as the second positional argument

I guess these 3 things could be split. Kept them together for convenience.

Question

Should we change the dataloader arguments to singular instead of plural?

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?
  • 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 feature Is an improvement or enhancement refactor labels May 7, 2021
@carmocca carmocca added this to the v1.4 milestone May 7, 2021
@carmocca carmocca self-assigned this May 7, 2021
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented May 7, 2021 •
edited
Loading

Hello @carmocca! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-15 11:26:10 UTC

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2021 •
edited
Loading

Codecov Report

Merging #7431 (8950e11) into master (024cf23) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7431   +/-   ##
======================================
- Coverage      92%     91%   -0%     
======================================
  Files         203     203           
  Lines       13151   13173   +22     
======================================
- Hits        12042   12026   -16     
- Misses       1109    1147   +38

@carmocca carmocca changed the title [WIP] Standardize positional datamodule and argument names Standardize positional datamodule and argument names May 7, 2021
@carmocca carmocca marked this pull request as ready for review May 7, 2021 15:49
@mergify mergify Bot added the has conflicts label May 7, 2021
@Borda Borda requested review from Borda and tchaton May 11, 2021 22:41
@mergify mergify Bot removed the has conflicts label May 13, 2021
@mergify mergify Bot removed the has conflicts label May 24, 2021
@mergify mergify Bot added the has conflicts label Jun 8, 2021
@carmocca carmocca added the ready to be merged PRs ready to be merged label Jun 11, 2021
@mergify mergify Bot removed the has conflicts label Jun 11, 2021
Borda
Borda reviewed Jun 12, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

consider using this wrapper https://github.com/Borda/pyDeprecate#self-argument-mapping
with mapping to rank zero https://github.com/PyTorchLightning/pytorch-lightning/blob/96433d03ea6323adea9a822a8222f7ea7ffba99b/pytorch_lightning/metrics/utils.py#L32

tchaton
tchaton approved these changes Jun 14, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM !

Comment thread docs/source/advanced/multiple_loaders.rst
@awaelchli awaelchli enabled auto-merge (squash) June 14, 2021 14:03
@awaelchli awaelchli merged commit 560b197 into master Jun 15, 2021
@awaelchli awaelchli deleted the feature/datamodule-as-arg branch June 15, 2021 11:50
@jorshi jorshi mentioned this pull request Aug 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Borda Borda Borda left review comments

@tchaton tchaton tchaton approved these changes

@justusschock justusschock justusschock approved these changes

@ananthsub ananthsub Awaiting requested review from ananthsub

@edenlightning edenlightning Awaiting requested review from edenlightning

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@edgarriba edgarriba Awaiting requested review from edgarriba

+1 more reviewer

@awaelchli awaelchli awaelchli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carmocca carmocca

Labels

feature Is an improvement or enhancement ready to be merged PRs ready to be merged refactor

Projects

None yet

Milestone

pl:1.4

Development

Successfully merging this pull request may close these issues.

7 participants

@carmocca @pep8speaks @edgarriba @awaelchli @Borda @tchaton @justusschock