Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refactor Model Zoo #166
Refactor Model Zoo #166
Changes from 18 commits
f3ebd0e
1a1308c
b3d60dd
e7913fd
11279a9
4fdd527
bbe3a81
0632b40
789a7ba
dd9053b
2841633
f2ba5ad
055168b
9ef6644
1339aac
8ed435a
105e280
bcd8f03
b850c47
dc0aecc
cad487e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 코드들과 일관성을 맞추면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dc0aecc 에서 반영했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postprocessor_type
의 타입을 보니Union[str, Platform]
로 되어있는데요. 값을str
타입으로 쓰고있는 이유가 무엇일까요?Platform
을 쓰는 것이 좋을 것 같은데, 특별한 이유가 있었을 것 같기도 해서요.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform 이 strEnum 이기도 하고, 사용자가 편하게 문자열로 선택하면 좋을 것 같아서 그렇게 했습니다. 사실 strict 하게 typing 하자면 Platform만 주는 것이 맞긴 한데요, 모델 주는 사용자 편의성이 중요한 프로젝트인 것 같아서 임의로 그렇게 했습니다. 아님 혹시 더 좋은 방법이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 커멘트는 가볍게 드리는 의견입니다. 다른 분들도 견해를 들려주시면 좋을 것 같습니다.
기존의
with_quantize
나 이번의skip_quantize
라는 이름을 보았을 때,quantize를 왜 해야하는지? 언제 skip 하는지? 어떤 효과를 주는지? 마음대로 설정할 수 있는지? 에 대한 의문이 생깁니다.
context를 아는 경우에는 조금만 생각해보면 알 수 있지만, 아닌 일반 사용자는 쉽게 이해할 수 있을지 모르겠네요.
저는 모델의 입력 텐서 타입이 float인지 int(uint)인지가 중요하지 않을까, 하고 생각이 들었는데요,
float 라면 image를 read한 값을 형변환을 할 것이고, uint라면 변환을 생략할 것이기 때문입니다.
결과적으로 quantize 여부는 타입에 의존적이게 되므로, 이를 인자로 True/False로 지정하여 선택이 가능한 것 처럼 보이기보다는
모델이 어떤 타입인지를 표현하는 인자가 되면 보다 설명이 간단해지지 않을까 싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cad487e 에서 with_scaling이라는 파라미터로 바꾸었고 설명을 추가했습니다. 구두로 논의 나눈 내용이라 자세히 적지는 않겠습니다. 감사합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 PR 이 크기 때문에 이 관련해서는 별도 PR 로 다루면 좋을 것 같기는 합니다만, 조금 설명을 듣고 싶은데요. with_scaling 이라는 이름이 여전히 모호한 느낌이 있는데요. 제가 인용한 병찬님이 말씀 처럼
use_fp32 = True
같은 옵션이나 dtype을 직접 넣는 것을 고려하지 않았는지 궁금하기도 합니다.