Revision models.detection.yolo

[redleaf-kim]

What does this PR do?

Related to #839

• [x] pl_bolts.models.detection.yolo.yolo_config.py
• [x] pl_bolts.models.detection.yolo.yolo_module.py
• [x] pl_bolts.models.detection.yolo.yolo_layers.py

Added some unit tests & yolo_giou config

• [x] tests.models.yolo.test_yolo_config.py
• [x] tests.models.yolo.test_yolo_layers.py
• [x] tests.data.yolo_giou.cfg

Feedback is welcomed with open arms.

Before submitting

• [x] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• [x] Did you read the contributor guideline, Pull Request section?
• [x] Did you make sure your PR does only one thing, instead of bundling different changes together?
• [x] Did you make sure to update the documentation with your changes?
• [x] Did you write any new necessary tests? [not needed for typos/docs]
• [x] Did you verify new and existing tests pass locally with your changes?
• [ ] If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

• [x] Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

[luca-medeiros]

Maybe we can consider moving with https://github.com/Lightning-AI/lightning-bolts/pull/817 too.

[otaj]

Hi, @heimish-kyma, do you plan to continue on this PR? :zap: Thank you! :nut_and_bolt:

[redleaf-kim]

Hi, @otaj sorry for being delayed first of all.

I will take care things mentioned above comments this weekends, Cheers!

[otaj]

Hi, @otaj sorry for being delayed first of all.

I will take care things mentioned above comments this weekends, Cheers!

@heimish-kyma, no need to apologize, I know things are taking a long time for me as well :smile: Thank you :zap:

[senarvi]

Maybe we can consider moving with #817 too.

@luca-medeiros I'm the author of that update. I'm sorry I didn't notice this pull request earlier. This PR seems to mostly contain new tests, which is very good, because the tests will always be useful. Otherwise, the YOLO code is old and has been improved quite a bit in my pull request. Better documented, complete type hints throughout the code, etc. I've also refactored it into multiple classes and added new features. Does it make sense to merge the tests from this PR and then move to https://github.com/Lightning-AI/lightning-bolts/pull/817 ? I mean, will the work be obsolete, if a lot of work is put into fixing things like the type hints in this PR?

[senarvi]

However, if I can ask for a bit more careful review of yolo_module.py file? First of all, I'd like to avoid using mutable objects as default arguments to methods as it can very easily come back and bite you. Another nitpick is that in infer() you are setting the model to eval() mode, but not back to the original mode after the method.

Plus, to be fair, I am not very big fan of the design of forward method (one big opaque ModuleList, that is always doing same checks such as which module is supposed to have which inputs, every single time in forward instead of doing this once in __init__ to have this constructed a bit more carefully. But, I absolutely understand you're not the original author of this implementation, so those complete redesigns might be a bit more difficult - and they're not the point of this review. But it's something I had to get off my chest 😂

Hi @otaj, good comments. I'm the original author, so I guess other than the catch_warnings fixture, these comments are related to my code. I fixed those small things in my pull request, i.e.

• removed unused module arguments,
• avoided mutable objects as default arguments, and
• infer() returns the model to the previous mode.

About the last comment: The design is ugly, because we read a Darknet configuration file and construct the network dynamically. My pull request adds support for using any typical PyTorch network that constructs the modules in __init__. That is, I have separated the Darknet network, which creates a ModuleList based on a Darknet configuration file, and the YOLO model, which can also accept traditional PyTorch networks.

If you have time to look at that pull request too, I would be interested to hear what you think.

[otaj]

@senarvi that is great to hear! If you could please merge your PR into this branch, that would be great! I would do it myself, but I have to say, I got a bit overwhelmed by the amount of changes in #817 and navigating them myself without knowing too much of the context felt like I could easily mess something up :joy: I believe that since you have more context about YOLO than I do, you'd be the perfect person to do so. Btw, nice tool you can use for it is gh (github-cli), that allows you to checkout a PR by number and registers branches and remotes for you.

The reason I'm asking you to do this is simple, I don't want to merge this PR without resolving these issues we brought up in review (i.e. mutable arguments, etc.) and since you're saying you've already solved them in your PR, that feels like the best course of action.

Thank you!

[senarvi]

@senarvi that is great to hear! If you could please merge your PR into this branch, that would be great!

@otaj sure, I can try to do that. But the source branch of this PR is in @heimish-kyma 's repository and presumably I don't have write access there. To which branch do you actually suggest that I write the merged code?

[senarvi]

@otaj I merged the changes, but as I guessed, I couldn't push them into this branch. I pushed them into my branch. So the merged changes appear now in this pull request. I don't know if that's what you meant. I'll now go through your comments there.

[senarvi]

@heimish-kyma should self.log(..., batch_size=images.size(0)) be used in test_step() too?