-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[microNPU] Add MobileNetv2 test to network tests #10242
Conversation
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.
Thanks @jacobbohlin! I left a few comments, let me know what you think
if __name__ == "__main__": | ||
test_forward_mobilenet_v1() | ||
test_forward_mobilenet_v1(ACCEL_TYPES[0]) |
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.
I think we could just remove this section completely?
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.
Agreed, that's probably better.
|
||
mod = partition_for_ethosu(relay_mod, params) | ||
compiled_models = infra.build_source( | ||
mod, input_data, output_data, accel_type, output_tolerance=10 |
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.
It would be interesting to see if the output tolerance could be lowered now that we support more operators
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.
I can give that a try.
@@ -65,5 +65,31 @@ def test_forward_mobilenet_v1(accel_type): | |||
infra.verify_source(compiled_models, accel_type) | |||
|
|||
|
|||
def test_forward_mobilenet_v2(accel_type="ethos-u55-256"): |
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.
Is it worth parameterising this network like the above?
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.
I cannot say I have a definitive answer for this. Probably not the most efficient way to improve test coverage relative to test time.
Thanks @jacobbohlin I think we could refactor the tests here to be a single one with stimulus being the model url and accelerator variant. Some references that we have done this at other places, also feel free to migrate any utility that you feel we can re-use to aot_test_utils tvm/tests/python/relay/aot/test_crt_aot_usmp.py Lines 205 to 219 in 0009a30
tvm/tests/python/relay/aot/test_crt_aot_usmp.py Lines 232 to 240 in 0009a30
|
Thanks @manupa-arm. That is a much better approach, I will update according to your suggestions. |
Co-authored-by: Jacob Bohlin <jacob.bohlin@arm.com>
82fcfb6
to
b4ecc38
Compare
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.
Thanks @jacobbohlin LGTM!
Closing this since it has been superseded by #10362. |
This adds MobileNetv2 tflite testcase to the microNPU's network tests.
Co-authored-by: Jacob Bohlin jacob.bohlin@arm.com