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

Revision models.detection.yolo #851

Merged
merged 32 commits into from
May 20, 2023

Conversation

redleaf-kim
Copy link
Contributor

What does this PR do?

Related to #839

  • pl_bolts.models.detection.yolo.yolo_config.py
  • pl_bolts.models.detection.yolo.yolo_module.py
  • pl_bolts.models.detection.yolo.yolo_layers.py

Added some unit tests & yolo_giou config

  • tests.models.yolo.test_yolo_config.py
  • tests.models.yolo.test_yolo_layers.py
  • tests.data.yolo_giou.cfg

Feedback is welcomed with open arms.

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 your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • 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

  • 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 🙃

@github-actions github-actions bot added the model label Aug 3, 2022
Copy link
Contributor Author

@redleaf-kim redleaf-kim left a comment

Choose a reason for hiding this comment

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

remove unused import & variable

@redleaf-kim redleaf-kim marked this pull request as ready for review August 3, 2022 13:14
Copy link
Contributor

@luca-medeiros luca-medeiros left a comment

Choose a reason for hiding this comment

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

Very thoughtful tests! I would re-check all type hints once more and make sure tests comply with #844. Good stuff 👍

Comment on lines 260 to 261
else:
assert False, "Unknown overlap loss: " + overlap_loss_name
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if that's the desired design. Maybe rather than raising an error, we could set IoU as default and raise a warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the code six lines above, the default should be mse

Copy link
Contributor

Choose a reason for hiding this comment

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

This was based on the original YOLO implementation, which I think uses MSE loss. That's why "mse" was the default. Darknet configuration files allow other variants of the iou loss (diou, ciou) that were not available in Torchvision at the time of writing this code, so I chose to use IoULoss in case something else than "mse" or "giou" is specified in the configuration file. I created another pull request a while ago that adds support for the ciou and diou losses, that are now available in Torchvision.

def _create_shortcut(config, num_inputs):
module = yolo_layers.ShortcutLayer(config["from"])
return module, num_inputs[-1]


@under_review()
def _create_upsample(config, num_inputs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Type hints. Would be nice to re-check all methods of this PR for type hints!

pl_bolts/models/detection/yolo/yolo_config.py Outdated Show resolved Hide resolved
tests/models/test_detection.py Outdated Show resolved Hide resolved
Comment on lines +37 to +38
if config["batch_normalize"]:
assert len(conv) == 3
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this part testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the conv layer doesn't use batch normalize, the length of module is 2 which is consist of conv and act layers.

tests/models/yolo/unit/test_yolo_config.py Outdated Show resolved Hide resolved
tests/models/yolo/unit/test_yolo_layers.py Show resolved Hide resolved
@luca-medeiros
Copy link
Contributor

Maybe we can consider moving with #817 too.

Copy link
Contributor

@luca-medeiros luca-medeiros left a comment

Choose a reason for hiding this comment

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

You can check #848 to have an idea of how to use the 'catch_warnings' on all your tests.

@@ -500,5 +500,5 @@ def __init__(self, source_layer: int) -> None:
super().__init__()
self.source_layer = source_layer

def forward(self, x, outputs):
def forward(self, x, outputs: List[Union[Tensor, None]]) -> Tensor:
Copy link
Contributor

Choose a reason for hiding this comment

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

We could review why we are passing 'x' to the forward method and doing nothing with it. Seems to be just to keep with the format...

Copy link
Contributor

Choose a reason for hiding this comment

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

@luca-medeiros you're right. RouteLayer and ShortcutLayer do not use x (the output from the previous layer) and calling them is anyway handled as a special case, so the x can be dropped.

@@ -483,7 +483,7 @@ def __init__(self, source_layers: List[int], num_chunks: int, chunk_idx: int) ->
self.num_chunks = num_chunks
self.chunk_idx = chunk_idx

def forward(self, x, outputs):
def forward(self, x, outputs: List[Union[Tensor, None]]) -> Tensor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this 'x' here!

Copy link
Contributor

@otaj otaj left a comment

Choose a reason for hiding this comment

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

Hi, @heimish-kyma, sorry for taking so long. The PR in general looks good, I agree with all of the luca-medeiros's comments. (redundant argument in couple places, not using catch_warnings fixture.

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 😂

tests/models/yolo/unit/test_yolo_layers.py Outdated Show resolved Hide resolved
tests/models/yolo/unit/test_yolo_layers.py Outdated Show resolved Hide resolved
tests/models/yolo/unit/test_yolo_config.py Outdated Show resolved Hide resolved
tests/models/yolo/unit/test_yolo_config.py Outdated Show resolved Hide resolved
tests/models/yolo/unit/test_yolo_config.py Outdated Show resolved Hide resolved
tests/models/yolo/unit/test_yolo_config.py Outdated Show resolved Hide resolved
tests/models/test_detection.py Outdated Show resolved Hide resolved
tests/models/test_detection.py Outdated Show resolved Hide resolved
Copy link
Contributor

@otaj otaj left a comment

Choose a reason for hiding this comment

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

Btw, mypy is showing some serious errors in typing. We don't necessarily need to get typing to 100%, but we don't want to introduce wrong typing

@otaj otaj mentioned this pull request Aug 12, 2022
@otaj
Copy link
Contributor

otaj commented Sep 15, 2022

Hi, @heimish-kyma, do you plan to continue on this PR? ⚡ Thank you! 🔩

@redleaf-kim
Copy link
Contributor Author

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

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

@otaj
Copy link
Contributor

otaj commented Sep 16, 2022

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 😄 Thank you ⚡

@senarvi
Copy link
Contributor

senarvi commented Oct 6, 2022

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
Copy link
Contributor

otaj commented Oct 11, 2022

@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 😂 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
Copy link
Contributor

senarvi commented Oct 11, 2022

@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
Copy link
Contributor

senarvi commented Oct 12, 2022

@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
Copy link
Contributor

senarvi commented Oct 12, 2022

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

@mergify mergify bot removed the has conflicts label Oct 27, 2022
@stale stale bot added the won't fix This will not be worked on label Dec 31, 2022
@stale stale bot closed this Jan 22, 2023
@Borda Borda reopened this Jan 24, 2023
@Lightning-Universe Lightning-Universe deleted a comment from stale bot Jan 24, 2023
@stale stale bot removed the won't fix This will not be worked on label Jan 24, 2023
@Borda Borda force-pushed the master branch 2 times, most recently from afdc09a to c5137b4 Compare May 19, 2023 17:15
@mergify mergify bot removed the has conflicts label May 19, 2023
@mergify mergify bot added the ready label May 19, 2023
@Borda Borda merged commit 8cb0a2d into Lightning-Universe:master May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants