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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added initial semantic segmentation example #751

Merged
merged 7 commits into from
Feb 16, 2020
Merged

added initial semantic segmentation example #751

merged 7 commits into from
Feb 16, 2020

Conversation

akshaykulkarni07
Copy link
Contributor

Before submitting

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

What does this PR do?

Fixes #635 (Semantic Segmentation example).

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.

Some additional info

I tried to add a test_step in the LightningModule which logs the predicted segmentation output. Since the default logger is TensorBoard, I expected the images to show up when I use tensorboard logdir=lightning_logs (at the default save path). But, they don't show up. I couldn't find any example which uses the logger properly. So, if someone can point me to an example, then I'll finish that (right now it's commented out).

Also, I have kind of followed the GAN example as a template, so I have put all of the code in a single file. If you prefer, then I can separate the Dataset class into another file. And I have added this as a full_example, so please let me know if you want it in domain_examples or something.

Also, I have used the KITTI dataset (since it is small and enough for an example) but it is not available directly in torchvision. So, it needs to be downloaded separately and the path needs to be given in the hparams. Note that the segmentation datasets available in torchvision (like Pascal VOC) are too large for a simple example.

Also, I couldn't figure out what documentation or tests are required, because none of the other examples have any documentation or tests (that I could find). Please point out if anything is required.

@Borda Borda added the feature Is an improvement or enhancement label Jan 31, 2020
@Borda Borda added this to the 0.6.1 milestone Jan 31, 2020
@williamFalcon
Copy link
Contributor

@akshaykvnit awesome. can you get the tests to pass and update the docs?

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.

Thx for this example, and apology for the delay...
Could you pls have look at comments and

  1. get the tests to pass
  2. add general docstring t the file root with reference to resources

@Borda
Copy link
Member

Borda commented Feb 13, 2020

@williamFalcon a bit more general thought is it would be better to write examples as ipython notebooks which we can transform to nice pages in Docs even with images as I did here

@akshaykulkarni07
Copy link
Contributor Author

@Borda @williamFalcon I have made most of the changes suggested by @Borda in the last review (with the exception of inference usage) and got the tests to pass.

I haven't been able to implement the inference usage properly. That will take some more time.

@williamFalcon a bit more general thought is it would be better to write examples as ipython notebooks which we can transform to nice pages in Docs even with images as I did here

I had initially implemented this in a Jupyter Notebook, but then the usage of hparams (using argparse and taking input from command line) won't be possible as they have been used in all your examples. So, if we need to convert the examples to iPython notebooks, then I could open a new PR, but do mention what can be done about the hparams.

@Borda
Copy link
Member

Borda commented Feb 14, 2020

I haven't been able to implement the inference usage properly. That will take some more time.

@akshaykvnit do you want to do it here in this PR or would you prefer to make a follow-up PR?

I had initially implemented this in a Jupyter Notebook, but then the usage of hparams (using argparse and taking input from command line) won't be possible as they have been used in all your examples. So, if we need to convert the examples to iPython notebooks, then I could open a new PR, but do mention what can be done about the hparams.

In this case, I have in mind to create a kind of lightning ZOO with the models and their particular trainers and then having notebook would be nice as an extra example with hparams can be simple derived without duplicating extra code... just building from prefabricated bricks... @williamFalcon ?

@akshaykulkarni07
Copy link
Contributor Author

@akshaykvnit do you want to do it here in this PR or would you prefer to make a follow-up PR?

Yes, I'll make a follow-up PR.

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.

Almost there 🚀 pls add documentation to the classes (better also to methods) as we are generating Docs from all examples...

pl_examples/full_examples/semantic_segmentation/semseg.py Outdated Show resolved Hide resolved


class SegModel(pl.LightningModule):
def __init__(self, hparams):
Copy link
Member

Choose a reason for hiding this comment

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

as examples are becoming part of the Docs, we shall add at least basic documentation with a brief description and reference to resources...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what documentation you need here. Because none of the other examples have documentation for the LightningModule class.

Copy link
Member

Choose a reason for hiding this comment

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

I ment someting like:

"""Image segmentation model.

This is a basic image segmentation model implemented with Lightning ...

References
- <link to source / publication>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the latest commit

@Borda
Copy link
Member

Borda commented Feb 15, 2020

@akshaykvnit do you want to do it here in this PR or would you prefer to make a follow-up PR?

Yes, I'll make a follow-up PR.

btw, just curious how did you verify without inference? just see the validation/test curve?

@pep8speaks
Copy link

pep8speaks commented Feb 16, 2020

Hello @akshaykvnit! Thanks for updating this PR.

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

Comment last updated at 2020-02-16 12:52:52 UTC

@akshaykulkarni07
Copy link
Contributor Author

akshaykulkarni07 commented Feb 16, 2020

@akshaykvnit do you want to do it here in this PR or would you prefer to make a follow-up PR?

Yes, I'll make a follow-up PR.

btw, just curious how did you verify without inference? just see the validation/test curve?

@Borda I implemented the reference using normal PyTorch and it works. You can see the results here in the last block of the notebook. I added the same code after trainer.fit(model) and it works properly. Thus, we can be sure that the training works.

However, I figured that you needed inference to work when trainer.test(model) is called. I haven't managed to properly implement in this fashion (some problem with the test_step and test_end functions). However, I shall try to fix that on my own (and open a new PR when done or open an issue if stuck).

@Borda
Copy link
Member

Borda commented Feb 16, 2020

@williamFalcon how about the docs for examples? I would say it shall contain them as it is part of Docs but probably it is not a blocker... :]

@williamFalcon williamFalcon merged commit 11db9d2 into Lightning-AI:master Feb 16, 2020
@williamFalcon
Copy link
Contributor

@akshaykvnit awesome tutorial!

@williamFalcon
Copy link
Contributor

williamFalcon commented Feb 17, 2020

@akshaykvnit @Borda

 ERROR collecting pl_examples/full_examples/semantic_segmentation/semseg.py

 No module named 'torchvision.models.segmentation'

oops, looks like this test fails. Need to fix asap

@Borda
Copy link
Member

Borda commented Feb 17, 2020

Yeah I have noticed that almost all examples are not tested at all... We shall run them with a minimal data....

@four4fish four4fish mentioned this pull request Feb 4, 2022
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semantic Segmentation example
4 participants