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

Extend and relax TensorList sample APIs #4358

Merged
merged 3 commits into from
Oct 18, 2022
Merged

Conversation

klecki
Copy link
Contributor

@klecki klecki commented Oct 14, 2022

Category: New feature, Bug fix

Description:

Add ResizeSample that allows to resize individual samples. Automatically converts the batch into non-contiguous mode, same as SetSample.

SetSample no longer requires the order between the batch and new sample to match - instead it waits for the incoming order in the order of current batch, so the access in the batch order will be correct.

Fix a problem that prohibited copying from empty batch that had no samples nor type set. Now it resets the current batch to similar state.

Those features are useful for implementing the Split and Merge operators.

Additional information:

Affected modules and functionalities:

TensorList

Key points relevant for the review:

If the parameters to order::wait are in correct order.

Tests:

  • 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

Sorry, something went wrong.

Add ResizeSample that allows to resize individual samples. Automatically
converts the batch into non-contiguous mode, same as SetSample.

SetSample no longer requires the order between the batch and new sample
to match - instead it waits for the incoming order in the order of
current batch, so the access in the batch order will be correct.

Fix a problem that prohibited copying from empty batch that had no
samples nor type set. Now it resets the current batch to similar state.

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Oct 14, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6183827]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6183827]: BUILD PASSED

@mzient mzient assigned mzient and unassigned szalpal Oct 17, 2022
// Bounds check
assert(sample_idx >= 0 && sample_idx < curr_num_tensors_);
// Resizing any individual sample converts the batch to non-contiguous mode
MakeNoncontiguous();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MakeNoncontiguous();
if (volume[new_shape] != volume[shape_.set_tensor_shape(sample_idx)])
MakeNoncontiguous();

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on slack, I think if we want to have a variant that doesn't break the contiguity, it should be a separate call like ReshapeSample or ResizeSample with some parameter, that would always enforce that we have a valid volume. That way the bahaviour is consistent and we know the postconditions of the call always.

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Oct 18, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6216588]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6216588]: BUILD PASSED

@klecki klecki merged commit 9d427d1 into NVIDIA:main Oct 18, 2022
@klecki klecki deleted the relaxed-samples branch October 18, 2022 17:30
@JanuszL JanuszL mentioned this pull request Jan 11, 2023
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