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

Add tests for operator cast. Revert to plain batched cast kernel until the optimized one is fixed. #3927

Merged
merged 3 commits into from
May 24, 2022

Conversation

mzient
Copy link
Contributor

@mzient mzient commented May 23, 2022

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Category:

Add tests for operator cast
Bug-fix (workaround) - use old cast kernel.

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

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

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

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4912461]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4912461]: BUILD FAILED

Comment on lines +94 to +122
print("At sample", i)
I = np.array(inp[i])
O = np.array(out[i])
R = ref[i]
print(I)
print(R)
print(O)
mask = np.logical_not(np.isclose(O, R, eps))
print("Differences at", mask)
print(I[mask])
print(R[mask])
print(O[mask])
print(np.count_nonzero(mask), "wrong values out of", mask.size)
assert np.array_equal(out[i], ref[i])
Copy link
Member

Choose a reason for hiding this comment

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

Whis would create some verbose printing in tests (every print is a new line). If we really need these prints, how about using print(f"...")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is printed only if a failure occurs.

Copy link
Member

Choose a reason for hiding this comment

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

I see. So still, how about making the code prettier by print(f"...")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make much difference as long as we actually want arguments to be separated by spaces...

else:
return x.astype(dtype)

def random_shape(rng, ndim:int, max_size:int):
Copy link
Member

Choose a reason for hiding this comment

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

I believe standard formatting of type hints is like:

def foo(myarg: int)

https://peps.python.org/pep-0484/


@nottest
def _test_operator_cast(ndim, batch_size, in_dtype, out_dtype, device):
src = lambda: generate(rng, ndim, batch_size, in_dtype, out_dtype)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Shouldn't the following be sufficient?

inp = fn.external_source(generate(...))

This lambda assignment looks suspicious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to pass a function, not the results.

Copy link
Member

Choose a reason for hiding this comment

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

How about making it more pythonic, by making a generate actually a generator and passing a generator object to external_source?

def generate(...):
   yield out

fn.external_source(generate(...))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is counterproductive - requires more work from me (writing a generator, specifying how to handle cycling) and from the operator (it will have to handle do a lot of inspection and will ultimately end up wrapping that generator in a yet another lambda).

Copy link
Contributor Author

@mzient mzient May 24, 2022

Choose a reason for hiding this comment

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

BTW, can you have an infinite generator? Because that's what I want here. Wrapping it in a generator that returns one item and cycling it doesn't seem any more pythonic than a lambda.
I guess one thing that would work instead of a lambda, is partial.

Copy link
Member

@stiepan stiepan May 24, 2022

Choose a reason for hiding this comment

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

can you have an infinite generator?

Do you mean something like that?

def gen():
  while True:
    yield batch

As to the pythonness I think the only objection is not generator over lambda but giving a name to a lambda.
https://peps.python.org/pep-0008/#programming-recommendations

Always use a def statement instead of an assignment statement that binds a lambda expression directly to an identifier:
# Correct:
def f(x): return 2*x
# Wrong:
f = lambda x: 2*x

else:
x
Copy link
Member

Choose a reason for hiding this comment

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

Does this else clause do anything? If not, is it needed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove.

@szalpal szalpal self-assigned this May 24, 2022
…e BinSearch is broken).

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
…e there.

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

CI MESSAGE: [4919803]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4919803]: BUILD PASSED

@JanuszL JanuszL merged commit d919d56 into NVIDIA:main May 24, 2022
stiepan pushed a commit that referenced this pull request May 24, 2022
…l the optimized one is fixed. (#3927)

* Add tests for operator cast. Revert to plain batched cast kernel (the BinSearch is broken).

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jun 7, 2022
…l the optimized one is fixed. (NVIDIA#3927)

* Add tests for operator cast. Revert to plain batched cast kernel (the BinSearch is broken).

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
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

5 participants