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

Change IR scale factor to 1.0 if tile size is too big #2337

Conversation

eugene123tw
Copy link
Contributor

@eugene123tw eugene123tw commented Jul 11, 2023

Summary

  • Set scale factor to 1.0 if tile size is larger than average image size
  • Change adaptive avgpool to avgpool to due ONNX tracing error:
TracerWarning: Converting a tensor to a Python integer might cause the trace to be incorrect. 
We can't record the data flow of Python values, so this value will be treated as a constant in the future. 
This means that the trace might not generalize to other inputs!
  k = [int(size[i] / output_size[i]) for i in range(0, len(size))]

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@github-actions github-actions bot added ALGO Any changes in OTX Algo Tasks implementation TEST Any changes in tests labels Jul 11, 2023
@eugene123tw eugene123tw self-assigned this Jul 11, 2023
@eugene123tw eugene123tw marked this pull request as ready for review July 12, 2023 13:50
@eugene123tw eugene123tw requested a review from a team as a code owner July 12, 2023 13:50
@jaegukhyun
Copy link
Contributor

It seems that this PR contents have changed from set ir_scale_factor to 1 if tile size is large to set default ir_scale_factor as 1.
Could you explain the background of this? @goodsong81 @sungmanc Is it okay to change default ir_scale_factor value?

@sungmanc
Copy link
Contributor

It seems that this PR contents have changed from set ir_scale_factor to 1 if tile size is large to set default ir_scale_factor as 1. Could you explain the background of this? @goodsong81 @sungmanc Is it okay to change default ir_scale_factor value?

I have a concern to change the default value. In the table below, we can easily check the Elapsed CPU time will be larger if we select the IR scale factor as 1. (5.28 -->2.87). Although IR FPS seems different, actually they are same since the number of tiles are different.
image

@sungmanc
Copy link
Contributor

@eugene123tw , @GalyaZalesskaya what do you think about the table above?

@goodsong81
Copy link
Contributor

From @GalyaZalesskaya's latest experiments, the effect of IR scale factor seems to have decreased after @sovrasov's async optimization, which essentially parallelize tile computation.

So now the ir_scale_factor is

  • error prone
  • no much speed up
  • make ModelAPI hard to generalize on tiling inference

In my opinion, it's OK to set 1 as default. Our, actually my original intention of "spatial concat" for tiles is to compensate the 1-size batch inference, and now async call is doing that.
@GalyaZalesskaya could you attach some summarized perf table here?

@eugene123tw
Copy link
Contributor Author

eugene123tw commented Jul 14, 2023

From @GalyaZalesskaya's latest experiments, the effect of IR scale factor seems to have decreased after @sovrasov's async optimization, which essentially parallelize tile computation.

So now the ir_scale_factor is

  • error prone
  • no much speed up
  • make ModelAPI hard to generalize on tiling inference

In my opinion, it's OK to set 1 as default. Our, actually my original intention of "spatial concat" for tiles is to compensate the 1-size batch inference, and now async call is doing that. @GalyaZalesskaya could you attach some summarized perf table here?

@sungmanc Songki summarised all the points. There's one point to add, we won't remove this parameter yet as it might have an impact on backward compatibility. And we can always switch it back if something unforeseen happens.

@GalyaZalesskaya GalyaZalesskaya mentioned this pull request Jul 14, 2023
8 tasks
@GalyaZalesskaya
Copy link
Contributor

From @GalyaZalesskaya's latest experiments, the effect of IR scale factor seems to have decreased after @sovrasov's async optimization, which essentially parallelize tile computation.

So now the ir_scale_factor is

  • error prone
  • no much speed up
  • make ModelAPI hard to generalize on tiling inference

In my opinion, it's OK to set 1 as default. Our, actually my original intention of "spatial concat" for tiles is to compensate the 1-size batch inference, and now async call is doing that. @GalyaZalesskaya could you attach some summarized perf table here?

Sure, here are the experiments on small data objects datasets and Vitens-Kiegetal datasets:
image
image

Yes, setting ir_scale_factor to 1 will increase the performance time, but for now the tiling accuracy is low and doesn't work well.

Copy link
Contributor

@sungmanc sungmanc left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation

@eugene123tw eugene123tw merged commit 2d8fd86 into openvinotoolkit:develop Jul 17, 2023
@eugene123tw eugene123tw deleted the eugene/CVS-114896-fix-ir-scale-factor branch July 17, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ALGO Any changes in OTX Algo Tasks implementation TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tile_size becomes bigger than an image size after convertion model to IR
6 participants