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.

Sign up for GitHub

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

Jump to bottom

feat(semseg): allow model customization #1371

Merged
merged 8 commits into from
Apr 16, 2020
Merged

feat(semseg): allow model customization #1371

merged 8 commits into from
Apr 16, 2020

Conversation

borisdayma
Copy link
Contributor

Replacing #1338 for rebasing

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?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Allow more customization to the semantic segmentation example:

  • test set has been replace with a validation set to measure model performance
  • both training and validation losses are logged
  • unet allows for more customization: number of layers and number of features
  • masks have been updated to be compatible with the most recent version of the dataset which contains more labels (additional labels are just ignored)
  • script allows for other arguments: distributed backend, 16 bit, number of layers and features for unet as well as type of upsampling, gradient accumulation, number of epochs, logging on W&B

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 🙃

@mergify mergify bot requested a review from a team April 4, 2020 06:05
@borisdayma borisdayma mentioned this pull request Apr 4, 2020
Closed
5 tasks
@codecov
Copy link

codecov bot commented Apr 4, 2020 •
edited
Loading

Codecov Report

Merging #1371 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1371   +/-   ##
======================================
  Coverage      91%     91%           
======================================
  Files          67      67           
  Lines        3778    3778           
======================================
  Hits         3433    3433           
  Misses        345     345

@Borda
Copy link
Member

Borda commented Apr 4, 2020

@borisdayma what is the diff to #1338 while it has the same name?

@borisdayma
Copy link
Contributor Author

From this comment I understood I had to rebase and make a new PR.

@Borda
Copy link
Member

Borda commented Apr 4, 2020 •
edited
Loading

From this comment I understood I had to rebase and make a new PR.

ok, it seems like a misunderstanding, rebase was sufficient, no need for creating new PR lol

@Borda Borda added feature Is an improvement or enhancement example labels Apr 5, 2020
@Borda Borda self-assigned this Apr 5, 2020
@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2020

This pull request is now in conflict... :(

@Borda Borda changed the title feat(semseg): allow model customization [blocked by #1419] feat(semseg): allow model customization Apr 8, 2020
@Borda Borda changed the title [blocked by #1419] feat(semseg): allow model customization feat(semseg): allow model customization Apr 8, 2020
@Borda
Copy link
Member

Borda commented Apr 8, 2020

changelog need to be rebased on new release #1419

@borisdayma
Copy link
Contributor Author

Sorry but I'm not sure of the process of rebasing a PR. Would github allow me to rewrite previous history? I'm typically used to merge commits.

@Borda
Copy link
Member

Borda commented Apr 8, 2020

yes, it does, and just when mentioned PR is merged I can do it for you so you do not need to do anything... :]

@pep8speaks
Copy link

pep8speaks commented Apr 16, 2020 •
edited
Loading

Hello @borisdayma! Thanks for updating this PR.

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

Comment last updated at 2020-04-16 14:15:17 UTC

Borda
Borda approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@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.

LGTM 🐰

@mergify mergify bot requested a review from a team April 16, 2020 14:13
@Borda Borda added the ready PRs ready to be merged label Apr 16, 2020
@Borda Borda added this to the 0.7.4 milestone Apr 16, 2020
@borisdayma
Copy link
Contributor Author

Thanks a lot for the changes!

@williamFalcon williamFalcon merged commit 06e6ead into Lightning-AI:master Apr 16, 2020
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Jun 7, 2020
@borisdayma @Borda @tullie
feat(semseg): allow model customization (Lightning-AI#1371)
4c2a135 
* feat(semantic_segmentation): allow customization of unet

* feat(semseg): allow model customization

* style(semseg): format to PEP8

* fix(semseg): rename logger

* docs(changelog): updated semantic segmentation example

* suggestions

* suggestions

* flake8

Co-authored-by: J. Borovec <jirka.borovec@seznam.cz>
@Borda Borda modified the milestones: 0.7.4, v0.7.x Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

Assignees

@Borda Borda

Labels
example feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.7.x
Development

Successfully merging this pull request may close these issues.
4 participants
@borisdayma @Borda @pep8speaks @williamFalcon