adding the YOLACT instance segmentation model

[Abdullah-Elkasaby]

[adrianboguszewski]

Thank you for your contribution. Before we do the review please do the self-check first (copy the list and tick the item when it's done):

• [ ] each function is described by docstrings and type hints
• [ ] notebook contains explicit descriptions and explanatory diagrams
• [ ] the notebook doesn't use any data (image, video, etc.) that is not CC4.0 licensed
• [ ] there is a README.md file in consistent style (look at other notebooks)
• [ ] the notebook is added to the main README
• [ ] there are no grammar, punctuation or typo issues (use any free tool for that e.g. Grammarly)
• [ ] there are no committed files besides notebook and readme (please use images or videos from data dir)
• [ ] your PR doesn't change any other notebooks
• [ ] all CI checks passed

[adrianboguszewski]

And it shouldn't be as submodule but committed directly :)

[Abdullah-Elkasaby]

@adrianboguszewski Thanks for the feedback, I will be working on the changes.

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[Abdullah-Elkasaby]

@adrianboguszewski I have made some changes and ran the treon CI checks and passed, could you have a look?

• [x] Each function is described by docstrings and type hints.
• [x] The notebook doesn't use any data (image, video, etc.) that is not CC4.0 licensed.
• [x] There is a README.md file in consistent style (look at other notebooks).
• [x] The notebook contains explicit descriptions and explanatory diagrams.
• [x] The notebook is added to the main README.
• [x] There are no grammar, punctuation, or typo issues (use any free tool for that, e.g., Grammarly).
• [x] There are no committed files besides the notebook and README (please use images or videos from the data directory).
• [x] Your PR doesn't change any other notebooks.
• [x] Treon CI checks passed.

here's a screenshot of running it locally and the fork repo has it run too.

[Abdullah-Elkasaby]

@adrianboguszewski the docker treon check faild, however the notebook that did was not mine. could you also provide hints to help with the remaining checks?

[adrianboguszewski]

Docker treon failing is not related to your work. However, the code check CI is. Please fix all flake8 bugs. You can run it locally to see where to improve the code style. See these instructions

[adrianboguszewski]

Two more comments:

• What is the utils directory? Do you need all of these utils?
• Descriptions are missing. The notebook is educational content, so it should contain an explanation of why it was done like that. See other notebooks for the reference

[Abdullah-Elkasaby]

Two more comments:

• What is the utils directory? Do you need all of these utils?
• Descriptions are missing. The notebook is educational content, so it should contain an explanation of why it was done like that. See other notebooks for the reference

The model does need all of those, the original model was even more complex and had multiple dependencies to multiple directories. Are a description in the readme file and comments in the notebook of the utils directory enough?

[Abdullah-Elkasaby]

@adrianboguszewski I have some changes and the code check passed, however there's some error regarding Binder that I don't get.

[adrianboguszewski]

No worries about Binder. However, we must solve the following issue somehow: Is there any other place where you can download the weights? Is this pointing to your Google Drive?

[adrianboguszewski]

This output also doesn't look good (like wrong output - mixed areas)

[Abdullah-Elkasaby]

@adrianboguszewski the drive is not mine, it's available on the the main repo of the model, I can set it up on my drive for now, however I don't know if that will be suitable. as for the mixed areas problem, I will try to to work on but I might need some help.

[adrianboguszewski]

If it's points to the main repo, please find a way to skip that access denied error

[adrianboguszewski]

@Abdullah-Elkasaby, any plans to continue working on this?

[Abdullah-Elkasaby]

any plans to continue working on this?

Yes, I do have plans to continue working on this project. However, please note that there might be some delays or gaps in my availability from time to time.

[adrianboguszewski]

Ok. So please look at the above comments and fix them first.

[Abdullah-Elkasaby]

Ok. So please look at the above comments and fix them first.

I think I have fixed most of them, I will be working on the output problem.

[Abdullah-Elkasaby]

@adrianboguszewski could you have a look at the output now ? also the failing checks are related to renaming the PR.

[Abdullah-Elkasaby]

@adrianboguszewski the failing code check is not related to my notebook, could you please check that? also the PR check is related to renaming the PR because I reopened the pull request I guess.

[adrianboguszewski]

@Abdullah-Elkasaby, thanks for the changes. It looks better. I like that notebook :)

However, please still consider the following points:

• Descriptions are still missing. I mean the descriptions between code cells. It would be best if you described above any cell what you're going to do
• You should use the org model drive, not yours. If it causes any problems, they have to be fixed first. You shouldn't use your own drive - it's a security vulnerability
• Don't worry about the PR labeler. It still doesn't work as expected
• Please remove unnecessary blank lines (those at the top and bottom of cells)
• Please remove all commented code. It should be either runnable (uncommented) or gone

[Abdullah-Elkasaby]

@adrianboguszewski I have made these changes, but the org drive was causing a problem for you locally which I don't know what is it about . It's now pointing to the original drive of the model.

[adrianboguszewski]

Ok. Two more things to do:

• there are still blank lines at the top or bottom of some code cells - please find them and remove
• thanks for the descriptions - please convert them from headings to paragraphs. Headings should separate bigger sections of related cells only

[Abdullah-Elkasaby]

@adrianboguszewski I have added these changes, but unfortunately the docker test is now failing, but it's not related to my notebook.

[adrianboguszewski]

Don't worry about anything not related to your changes.

• The gitignore file doesn't seem to be needed in this PR, does it?
• We're avoiding committing new image files. The image with results should be an outcome from the code, not loaded from disk.

[Abdullah-Elkasaby]

Don't worry about anything not related to your changes.

• The gitignore file doesn't seem to be needed in this PR, does it?
• We're avoiding committing new image files. The image with results should be an outcome from the code, not loaded from disk.

the image output is notebook is not from disk, it's coming from the model, however that image on the disk is used as a thumbnail in the readmefile, I will delete it and use the image from a seperate repo.

I used the gitignore file to keep the downloaded model and the weights local as they are downloaded from google drive, I can delete it though

[adrianboguszewski]

Don't worry about anything not related to your changes.

• The gitignore file doesn't seem to be needed in this PR, does it?
• We're avoiding committing new image files. The image with results should be an outcome from the code, not loaded from disk.

the image output is notebook is not from disk, it's coming from the model, however that image on the disk is used as a thumbnail in the readmefile, I will delete it and use the image from a seperate repo.

In README files we use the links to images uploaded to GitHub. See the contribution guide.

I used the gitignore file to keep the downloaded model and the weights local as they are downloaded from google drive, I can delete it though

Gitignore is not needed to ignore those files. You can just skip them when committing.

It looks good to me. Thanks! I'm approving it and passing it to the next reviewer :)

[eaidova]

@Abdullah-Elkasaby generally, notebook is good, but do you really need so many files and manual model construction inside notebook? Can we just clone model repo and install it instead?

[eaidova]

please also rename notebook to keep the same name like notebook subfolder

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

eaidova commented on 2023-07-03T16:30:38Z ----------------------------------------------------------------

Line #1.    !pip install cython

please add -q for installing dependencies in quite mode