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

New warp example #1158

Merged
merged 3 commits into from
Oct 30, 2019
Merged

New warp example #1158

merged 3 commits into from
Oct 30, 2019

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Aug 9, 2019

Why we need this PR?

Pick one

  • It adds an example (and a kind of test) for NewWarpAffine
  • It fixes outstanding issues in CPU Argument Inputs
  • It adds randomized angle to Rotate test

What happened in this PR?

  • What was changed, added, removed?
  • the warp affine example with parameters passed in GPU memory
  • the warp affine example with parameters passed as named input (yay!)
  • added concat_str to join values into one string without delimiters
  • Was this PR tested? How?
  • it is a test
  • Were docs and examples updated, if necessary?
  • it is a doc

JIRA TASK: [DALI-1010]

const OpSpec &Spec() const { return this->spec_; }

private:
/// @defgroup WarpStaticType Dynamic to static type routing
Copy link
Contributor

Choose a reason for hiding this comment

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

Please adjust comments from /// -> /* */.


protected:
inline cudaStream_t GetStream() const {
return ws_ && ws_->has_stream() ? ws_->stream() : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use #1178

@@ -0,0 +1,164 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to make it a part of our example set please add more description why certain things are done.

@mzient mzient force-pushed the NewWarpExample branch 3 times, most recently from 7b72325 to e652a13 Compare October 28, 2019 17:02
@mzient mzient requested a review from JanuszL October 28, 2019 17:03
@mzient
Copy link
Contributor Author

mzient commented Oct 28, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [964451]: BUILD STARTED

* matrix as GPU argument (GPU variant) and as named input (CPU variant)
* different interpolation and border handling.
* add `concat_str` as a shortcut for `make_string_delim("", ...)
* minor fixes related to CPU ArgumentInputs

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Copy link
Contributor Author

mzient commented Oct 28, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [964461]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [964461]: BUILD PASSED

"db_folder = os.path.join(test_data_root, 'db', 'lmdb')\n",
"\n",
"def gen_transform(angle, zoom, dst_cx, dst_cy, src_cx, src_cy):\n",
" t1 = np.array([[1, 0, -dst_cx], [0, 1, -dst_cy], [0, 0, 1]])\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reformat this matrices to make them more readable.

" t2 = np.array([[1, 0, src_cx], [0, 1, src_cy], [0, 0, 1]])\n",
" return (np.matmul(t2, np.matmul(r, t1)))[0:2,0:3]\n",
"\n",
"def gen_transforms(n, step):\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

gen_transforms and gen_transform sounds very alike.

@@ -0,0 +1,172 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add more narrative with the detailed description of what is happening here. Now it is not something we can call a tutorial.
You can also show how to achieve things that were not possible earlier, like Shear or Translate operator.
Also please add this notebook to our docs (update appropriate *.rst inside docs folder to include this file).

* Chance make_string to use empty delimiter by default.
* Add `operator<<(ostream &, const TensorLayout&)`

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Copy link
Contributor Author

mzient commented Oct 29, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [965949]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [965949]: BUILD PASSED

@szalpal
Copy link
Member

szalpal commented Oct 29, 2019

I'd like to request a change to make the example better. Here's the list of things in my opinion are lacking:

  1. What actually is a Warp operation. And affine (yes, at least few words about it IMHO is necessary)
  2. What is the test dataset we are using
  3. What does it mean to define affine transform? (e.g. what type should it have? np.array? np.matrix?)
  4. Why the pipeline is called HybridPipe?
  5. Why we use CaffeReader in example about WarpAffine? Where's a caffe here?
  6. Code indentation and comment formatting
  7. What we actually expect to obtain from such pipeline?
  8. The code in the last snippet is fujka. Can we refactor it? Like close some of this in functions with meaningful names or sth. This code is for everyone to see...
  9. I guess not all the imports are necessary

Lastly, whole point of notebook is to use snippets + markdown descriptions. So I'd move in-code comments to the descriptions.

@mzient
Copy link
Contributor Author

mzient commented Oct 30, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [967690]: BUILD STARTED

@mzient
Copy link
Contributor Author

mzient commented Oct 30, 2019

!build

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [967754]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [967754]: BUILD PASSED

@mzient mzient merged commit b5afda9 into NVIDIA:master Oct 30, 2019
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