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

Update thresholds for ParameterSweepTest. #3710

Open
wants to merge 1 commit into
base: master
from

Conversation

@jsubag
Copy link
Contributor

jsubag commented Oct 31, 2019

Summary: Updating thresholds for FC, BatchMatMul and Conv sweep tests.

@jsubag

This comment has been minimized.

Copy link
Contributor Author

jsubag commented Oct 31, 2019

@jfix71 , @arunm-git Please take a look.

@jsubag

This comment has been minimized.

Copy link
Contributor Author

jsubag commented Oct 31, 2019

CC: @omromano

@jfix71

This comment has been minimized.

Copy link
Contributor

jfix71 commented Oct 31, 2019

@hyuen Can you take a look here?

@hyuen

This comment has been minimized.

Copy link

hyuen commented Nov 5, 2019

@jfix71 and I met and discussed about this. It seems right now that we are comparing fp32 as a baseline vs a quantized approach. The error threshold is dependent on the dimensions of the inputs, and by changing these thresholds (extending them) we are just accepting the biggest error which comes from the biggest dimension.

The proposed fix is to compare with a baseline in the same precision, and expect bitwise accuracy for most of the cases, that way we can flush out any differences in the quantization or accumulator schemes.

@jsubag

This comment has been minimized.

Copy link
Contributor Author

jsubag commented Nov 6, 2019

@hyuen I'm all for comparing with a more accurate reference - but skeptical about bitwise comparisons for non-integer arithmetic (especially across different HW).
Can you share more details about your reference model?

@stale

This comment has been minimized.

Copy link

stale bot commented Nov 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

facebook-github-bot added a commit that referenced this pull request Dec 3, 2019
… better bounds checking/testing (#3807)

Summary:
This PR introduces a `FoldElemKindConversionIntoIO` pass which looks for single-use Placeholders that have ConvertTo/Quantize/Dequantize right after/before them and folds them into the Placeholder (and SaveNode if an output). It then adds usage of this pass to `compareAgainstInterpreter()` tests. By doing this we allow the backend to more directly test the operator in its desired precision instead of also allowing the results of the test to be impacted by the specific logic used for type conversion (see bottom for before/after Functions).

This is opt-in, and at least initially meant for better bounds testing on tests which use `compareAgainstInterpreter()`. This is a dangerous pass, as it requires converting the associated Tensors for these Placeholders in PlaceholderBindings, and correctly getting handles to these Tensors based on these types. I've made this opt-in based on the optimization options in the compilation context.
Pull Request resolved: #3807

Test Plan:
All tests still pass. I modified the one test (BackendCorrectnessTest's `basicFCNetQuantized`) which currently does Int8 to Int8 comparison to expect bitwise accuracy.

Related to #3710

Before:
<img width="399" alt="before" src="https://user-images.githubusercontent.com/1198212/69445162-a8baca00-0d06-11ea-9082-30616feae3f4.png">

After:
<img width="510" alt="after" src="https://user-images.githubusercontent.com/1198212/69445173-ad7f7e00-0d06-11ea-88df-62c3635622ee.png">

Differential Revision: D18694602

Pulled By: jfix71

fbshipit-source-id: ff7da1704bc2dc23f380fbdc34eee0f01ef9e939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.