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

Detection head re-implementation for PyNetsPresso compatibility #194

Merged
merged 12 commits into from
Oct 16, 2023

Conversation

illian01
Copy link
Collaborator

Description

Please include a summary in English, of the changes in this pull request. If it closes an issue, please mention it here.

Closes: ~

You should link at least one existing issue for PR. Before your create a PR, please check to see if there is an issue for this change.
PRs from forked repository not accepted.

Change(s)

  • Separate model backbone and head
  • Revert NMS code to receive various batch size
  • Fix metric
  • Add validation loss computation

Changelog

If you PR to dev branch, please add a brief summary of the change to the Upcoming Release section of the CHANGELOG.md file and include a link to the PR (formatted in markdown) and a link to your github profile.

For example,

- Added a new feature by `@myusername` in [PR 2023](https://github.com/Nota-NetsPresso/netspresso-trainer/pull/2023)

Code Formatting

If you PR to either master or dev branch, you should follow the code linting process. Please check your code with lint_check.sh in ./scripts directory.
For more information, please read the contribution guide in CONTRIBUTING.md.

@illian01 illian01 self-assigned this Oct 13, 2023
@illian01 illian01 requested a review from deepkyu as a code owner October 13, 2023 08:56
Copy link
Contributor

@deepkyu deepkyu left a comment

Choose a reason for hiding this comment

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

@illian01
Thx for the update.
It seems that the metric calibration is updated as batch-wise calibration.
In later, it would be better not to use for-loop in mini-bach.

Plz check the other comments.

pred.append(pred_on_image)
self.metric_factory(pred, target=targets, phase='valid')

def save_checkpoint(self, epoch: int):
Copy link
Contributor

Choose a reason for hiding this comment

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

@illian01

If we override task-specific save_checkpoint function, it may be better to express as abstractmethod in base pipeline.
Someone may be hard to find where it is defined..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@deepkyu
I overrided save_checkpoint method because we have to separate backbone and head for detection task.
Thus, this overrided method don't need if we serve fx model with integrated one.
I think it would be better to remove this method when we can compress and benchmark the integrated detection task model.
I will fix save_checkpoint to abstractedmethod if we determined to serve detection model as is (backbone and head separately) through internal discussion.

pred_on_image['post_scores'] = detection[..., -1]
pred_on_image['post_labels'] = class_idx
pred.append(pred_on_image)
self.metric_factory(pred, target=targets, phase='valid')
Copy link
Contributor

Choose a reason for hiding this comment

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

@illian01

I updated to call with calc, instead of __call__. Originally, it called with the builtin __call__ as you modified, but in later it is hard to catch whether the metric_factory is method of the class or attribute of the class.

I added calc in later, but leave __call__ just for backward support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@deepkyu
I got it. I will fix to call self.metric_factory.calc method.

@illian01 illian01 requested a review from deepkyu October 16, 2023 02:25
@illian01 illian01 merged commit 50ae650 into dev Oct 16, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants