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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed datamodule from an input parameter #270

Merged
merged 7 commits into from Oct 15, 2020
Merged

Removed datamodule from an input parameter #270

merged 7 commits into from Oct 15, 2020

Conversation

deng-cy
Copy link
Contributor

@deng-cy deng-cy commented Oct 13, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Removed datamodule as an input parameter of MoCo model. There are two reasons:

  1. Datamodule is preferably passed into Train.fit() function.
  2. In the original code, datamodule will be saved as a hyperparameter, which makes the yaml difficult to read.

Besides, logging code was reformatted to match Lightning 1.0.0.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 馃檭

@pep8speaks
Copy link

pep8speaks commented Oct 13, 2020

Hello @deng-cy! Thanks for updating this PR.

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

Comment last updated at 2020-10-15 12:34:15 UTC

@mergify mergify bot requested a review from Borda October 13, 2020 04:03
@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #270 into master will increase coverage by 3.44%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   80.41%   83.86%   +3.44%     
==========================================
  Files          91       91              
  Lines        4840     4858      +18     
==========================================
+ Hits         3892     4074     +182     
+ Misses        948      784     -164     
Flag Coverage 螖
#cpu 23.91% <0.00%> (+<0.01%) 猬嗭笍
#pytest 23.91% <0.00%> (+<0.01%) 猬嗭笍
#unittests 83.32% <90.90%> (+3.45%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
..._bolts/models/self_supervised/moco/moco2_module.py 79.23% <87.50%> (+35.13%) 猬嗭笍
pl_bolts/models/vision/image_gpt/igpt_module.py 96.15% <100.00%> (+49.90%) 猬嗭笍
pl_bolts/utils/arguments.py 96.15% <0.00%> (+0.04%) 猬嗭笍
pl_bolts/losses/self_supervised_learning.py 77.70% <0.00%> (+0.14%) 猬嗭笍
pl_bolts/models/rl/common/gym_wrappers.py 89.91% <0.00%> (+0.26%) 猬嗭笍
pl_bolts/datasets/imagenet_dataset.py 19.16% <0.00%> (+0.37%) 猬嗭笍
pl_bolts/optimizers/lars_scheduling.py 95.74% <0.00%> (+0.50%) 猬嗭笍
pl_bolts/datasets/ssl_amdim_datasets.py 74.32% <0.00%> (+1.08%) 猬嗭笍
pl_bolts/models/self_supervised/resnets.py 92.78% <0.00%> (+2.57%) 猬嗭笍
...l_bolts/models/rl/vanilla_policy_gradient_model.py 94.64% <0.00%> (+2.67%) 猬嗭笍
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update bdfe159...2be5a6f. Read the comment docs.

@deng-cy deng-cy marked this pull request as ready for review October 13, 2020 17:37
@Borda
Copy link
Member

Borda commented Oct 15, 2020

@deng-cy great! can we do the same also for iGPT :]

@Borda Borda added fix fixing issues... enhancement New feature or request ci/cd Continues Integration and delivery labels Oct 15, 2020
@Borda Borda changed the title Removed datamodule from an input parameter of MoCo model Removed datamodule from an input parameter Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd Continues Integration and delivery enhancement New feature or request fix fixing issues...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants