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

Improve handling of "dtype" arguments in OpSchema/OpSpec #3981

Merged
merged 7 commits into from
Jun 14, 2022

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Jun 13, 2022

Signed-off-by: Michał Zientkiewicz mzient@gmail.com

Category:

Bug fix (non-breaking change which fixes an issue)
Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

Bug:
removing a default value from AddOptionalArg("name", "doc", value, TYPE_ID) resulted in adding an argument of type DALIDataType and the default value TYPE_ID. This happened in "fill_value" argument to WarpAttr. This PR fixes that.

Refactoring:

  • The type arguments added with AddOptionalArg are now added with AddOptionalTypeArg.
  • Mandatory "dtype" arguments are added with AddTypeArg
  • The problemantic overload was blocked with a static assertion and replaced with a dedicated AddOptionalTypeArg (and AddTypeArg) which is always of type DALIDataType.
  • The default value DALI_NO_TYPE was removed from existing operators - now it's simply "not present".
  • The GetArgument was replaced with TryGetArgument in places where the default value was removed and relied upon

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Existing tests: all over the place
New tests: test_operator_warp - added floating point fill value

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

…ck ambiguous overload of AddOptionalArg with a static_assert.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Don't pass "dtype" to warp_affine when it's the same as input - let the
operator infer the type.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5072940]: BUILD STARTED

@NVIDIA NVIDIA deleted a comment from dali-automaton Jun 13, 2022
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5072940]: BUILD FAILED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5073962]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5073962]: BUILD FAILED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@@ -244,7 +261,7 @@ def test_gpu_vs_cpu():
inv_map=inv_map)
gpu_pipeline.build()

compare(cpu_pipeline, gpu_pipeline, 1)
compare(cpu_pipeline, gpu_pipeline, 1.0001)
Copy link
Contributor Author

@mzient mzient Jun 14, 2022

Choose a reason for hiding this comment

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

The new fill value results in max differences slightly >1 (rounding + some error accumulation, likely result of using fma on GPU and plain mul/add on CPU).

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5079728]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5079728]: BUILD PASSED

out_of_bounds_policy_(GetOutOfBoundsPolicy(spec)) {
spec.TryGetArgument(output_type_, "dtype");
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked there's a default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, line 93.

@mzient mzient merged commit b4a46e1 into NVIDIA:main Jun 14, 2022
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.

None yet

4 participants