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.

Sign up for GitHub

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

Jump to bottom

Tensor concatenation and stacking #2350

Merged

mzient merged 4 commits into NVIDIA:master from mzient:TensorJoinOps

Oct 13, 2020

Contributor

mzient commented Oct 12, 2020

Why we need this PR?

Pick one, remove the rest

It adds new feature: tensor concatenation and stacking operators

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
- Use TensorJoinCPU/TenosrJoinGPU kernels in the operator
- Use a simple copy/reshaed copy when there's just 1 input
- Use any to store type-specific data in the operator and avoid writing full-blown pImpl
- Use function overloads to provide backend-specific RunImpl and have just one class
- Register the operator with different template arguments (backend and new_axis) as Cat/Stack CPU/GPU
Affected modules and functionalities:
- TensorJoin operator
- Constant operator (minor fix)
Key points relevant for the review:
- Axis handling
- Use of any for inputs
Validation and testing:
- Python tests
Documentation (including examples):
- Docstrings
- Jupyter

JIRA TASK: DALI-1620

mzient requested a review from a team

October 12, 2020 19:51

review-notebook-app bot commented Oct 12, 2020

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

mzient force-pushed the TensorJoinOps branch from 1e49a39 to 896748e Compare

October 13, 2020 10:11

mzient added 3 commits

October 13, 2020 15:24


          [WIP]

0ddc045

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


          It compiles!

d6c52e1

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


          Working!

c3bc56b

Added tests.
Added special handling for a simple copy (cat) and reshaped copy (stack).
Added a jupyter notebook with examples.
Removed unnecessary assert from Constant op.

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

mzient force-pushed the TensorJoinOps branch from 896748e to c3bc56b Compare

October 13, 2020 13:24

Contributor Author

mzient commented Oct 13, 2020

!build

Collaborator

dali-automaton commented Oct 13, 2020

CI MESSAGE: [1697570]: BUILD STARTED

jantonguirao approved these changes

View reviewed changes

docs/examples/general/tensor_join.ipynb Outdated

+                 "source": [
+                  "# Tensor joining\n",
+                  "\n",
+                  "This notebook demonstrates two metbhods of joining tensors: stacking and concatenation.\n",

Contributor

jantonguirao Oct 13, 2020

Suggested change

      
                "This notebook demonstrates two metbhods of joining tensors: stacking and concatenation.\n",
          
                "This notebook demonstrates two methods of joining tensors: stacking and concatenation.\n",

docs/examples/general/tensor_join.ipynb Outdated

+                  "\n",
+                  "Both of these operations take multiple inputs and produce the output by joining the input tensors.\n",
+                  "The difference between these methods is that concatenation joins the tensors along an existing axis, whereas stacking inserts a new axis.\n",
+                  "Stacking can be used, for example, to combine separate combine separate coordinates into vectors, or to combine color planes into color images.\n",

Contributor

jantonguirao Oct 13, 2020

Suggested change

      
                "Stacking can be used, for example, to combine separate combine separate coordinates into vectors, or to combine color planes into color images.\n",
          
                "Stacking can be used, for example, to combine separate coordinates into vectors, or to combine color planes into color images.\n",

dali/test/python/test_operator_join.py

+                      src2 = dali.types.Constant(np.array(
+                          [[],
+                           [],
+                           []], dtype=np.int32))

Contributor

jantonguirao Oct 13, 2020

Is it in purpose that only src2 has a dtype?

Contributor Author

mzient Oct 13, 2020

Others have it inferred from the values - here there are no values and it defaulted to float.

dali/operators/generic/join.cc Outdated

+                out.SetLayout(output_layout_);
+                TYPE_SWITCH(out.type().id(), type2id, T, TENSOR_JOIN_TYPES, (
+                  RunTyped(view<T>(out), ws);
+                ), (DALI_FAIL("Internal error: unsupported type reached RunImpl function")));  // NOLINT

Contributor

jantonguirao Oct 13, 2020

print the type?

Contributor Author

mzient Oct 13, 2020

If it was ever reached, then printing the type would shift attention from the real problem: that we've encountered a type that should have been rejected in Setup.

Contributor

JanuszL Oct 13, 2020

I don't think the user cares how fatal the error is, and the more debug info we get from the user the better.

dali/operators/generic/join.cc

+              This argument is mutually exclusive with ``axis``.
+              This argument requires that at least one input has a non-empty layout and that all non-empty
+              input layouts match.)", nullptr, false)
+                .NumInput(1, 999)

Contributor

jantonguirao Oct 13, 2020

999? Just checking, did you mean 99 (I've seen it as a limit to other operators). By the way, why not 100 or 1000?

Contributor

JanuszL Oct 13, 2020

Maybe we should have a define for an infinite number of inputs (999 + 1)?

Contributor Author

mzient Oct 13, 2020

That's a good idea, but maybe not for this PR...?

Contributor

JanuszL Oct 13, 2020

Sure

dali/operators/generic/join.cc Outdated

+                .AddOptionalArg<int>("axis", R"(The axis in the output tensor along which the inputs are stacked.
+              The axis is inserted before a corresponding axis in the inputs. A value of 0 indicates that whole
+              tensors are stacked. Speicfying ``axis`` equal to the number of dimensions in the inputs causes

Contributor

jantonguirao Oct 13, 2020

Suggested change

      
            tensors are stacked. Speicfying ``axis`` equal to the number of dimensions in the inputs causes
          
            tensors are stacked. Specifying ``axis`` equal to the number of dimensions in the inputs causes

dali/operators/generic/join.cc Outdated

+              the values from the inputs to be interleaved)", 0, false)
+                .AddOptionalArg<string>("axis_name", R"(Name of the new axis to be inserted.
+              A one-character that will denot the new axis in the output layout. The output layout will be

Contributor

jantonguirao Oct 13, 2020

Suggested change

      
            A one-character that will denot the new axis in the output layout. The output layout will be
          
            A one-character that will denote the new axis in the output layout. The output layout will be

dali/operators/generic/join.cc Outdated

+                .AddOptionalArg<string>("axis_name", R"(Name of the new axis to be inserted.
+              A one-character that will denot the new axis in the output layout. The output layout will be
+              constructed by inserting that character into the input layout at position indicated by ``axis``.

Contributor

jantonguirao Oct 13, 2020

Suggested change

      
            constructed by inserting that character into the input layout at position indicated by ``axis``.
          
            constructed by inserting that character into the input layout at the position indicated by ``axis``.

dali/operators/generic/join.cc Outdated

+              template <typename Backend, bool new_axis>
+              void TensorJoin<Backend, new_axis>::SetupAxis() {
+                // axis_name indicates the join axis for concatenation only;
+                // for stacking, it's the name of the new axiis

Contributor

jantonguirao Oct 13, 2020

Suggested change

      
              // for stacking, it's the name of the new axiis
          
              // for stacking, it's the name of the new axis

Collaborator

dali-automaton commented Oct 13, 2020

CI MESSAGE: [1697570]: BUILD FAILED

JanuszL reviewed

View reviewed changes

dali/operators/generic/join.cc Outdated

+                SetupAxis();
+                SetOutputLayout(ws);
+                // Run ove inputs and store them in a vector

Contributor

JanuszL Oct 13, 2020

Suggested change

      
              // Run ove inputs and store them in a vector
          
              // Run over inputs and store them in a vector

JanuszL reviewed

View reviewed changes

dali/operators/generic/join.cc Show resolved Hide resolved

JanuszL reviewed

View reviewed changes

docs/examples/general/tensor_join.ipynb

		@@ -0,0 +1,404 @@
		{

Contributor

JanuszL Oct 13, 2020

metbhods -> methods

combine separate combine separate coordinates into vectors -> combine separate coordinates into vectors

Reply via ReviewNB

mzient commented

View reviewed changes

dali/operators/generic/join.cc Show resolved Hide resolved


          Fix review issues. Fix clang build.

d1ec5d7

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

mzient force-pushed the TensorJoinOps branch from a3f0dd7 to d1ec5d7 Compare

October 13, 2020 15:25

JanuszL approved these changes

View reviewed changes

jantonguirao approved these changes

View reviewed changes

Contributor Author

mzient commented Oct 13, 2020

!build

mzient changed the title ~~Tensor join ops~~ Tensor concatenation and stacking

Collaborator

dali-automaton commented Oct 13, 2020

CI MESSAGE: [1697830]: BUILD STARTED

Collaborator

dali-automaton commented Oct 13, 2020

CI MESSAGE: [1697830]: BUILD PASSED

mzient merged commit 296663c into NVIDIA:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment