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

Gridmask Cpu #2582

Merged
merged 16 commits into from
Jan 25, 2021
Merged

Gridmask Cpu #2582

merged 16 commits into from
Jan 25, 2021

Conversation

PawelA
Copy link
Contributor

@PawelA PawelA commented Jan 2, 2021

Why we need this PR?

  • It adds new feature useful for EfficientDet pipeline

What happened in this PR?

  • What solution was applied:
  • Affected modules and functionalities:
    • Self-contained
  • Key points relevant for the review:
    • Kernel and operator
  • Validation and testing:
    • Added operator test
  • Documentation (including examples):
    • Todo

JIRA TASK: NA

@PawelA PawelA changed the title Gridmask Gridmask Cpu Jan 2, 2021
dali/kernels/mask/grid_mask_cpu.h Outdated Show resolved Hide resolved
dali/operators/image/mask/grid_mask.cc Outdated Show resolved Hide resolved
dali/operators/image/mask/grid_mask.cc Outdated Show resolved Hide resolved
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
float fx = -sx + y * -sa + x * ca;
float fy = -sy + y * ca + x * sa;
auto m = (fx - floor(fx) >= ratio) || (fy - floor(fy) >= ratio);
for (int c = 0; c < in.shape[2]; c++)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what would be the perf gain when C is a template argument and we have a compile time generated variant for 1, 3, 4 and the generic one like here.
@mzient what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The gain can be significant.

Signed-off-by: Paweł <pawel.anikiel@gmail.com>
Signed-off-by: Paweł <pawel.anikiel@gmail.com>
@JanuszL
Copy link
Contributor

JanuszL commented Jan 13, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1978194]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1978194]: BUILD PASSED

Signed-off-by: Paweł <pawel.anikiel@gmail.com>
@JanuszL
Copy link
Contributor

JanuszL commented Jan 18, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1990925]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1990925]: BUILD FAILED

@JanuszL
Copy link
Contributor

JanuszL commented Jan 18, 2021

CI test failed:

======================================================================
FAIL: test_operator_gridmask.test_cpu_vs_cv_random(<nvidia.dali.backend_impl.TensorCPU object at 0x7f9c1680e068>, <nvidia.dali.backend_impl.TensorCPU object at 0x7f9c1680e110>, 51, 0.38158387, 2.6810782)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/opt/dali/dali/test/python/test_operator_gridmask.py", line 90, in check
    assert np.all(result2 == input2)
AssertionError
----------------------------------------------------------------------

Can you check it?

@PawelA
Copy link
Contributor Author

PawelA commented Jan 18, 2021

I managed to reproduce it, the problem seems to be that tile * ratio (the width of black squares) is far from an integer (19.46), which trips up the grid generation (because first it makes an unrotated grid, then it rotates it). I can either improve the grid generation, or choose ratio so that tile * ratio is an integer. What do you think?

@JanuszL
Copy link
Contributor

JanuszL commented Jan 18, 2021

choose ratio so that tile * ratio is an integer

I don't think we can make such assumption in general. How the paper deals with that (or silently ignores the problem)?

improve the grid generatio

I think that would be the preferable way.

Signed-off-by: Paweł <pawel.anikiel@gmail.com>
@PawelA
Copy link
Contributor Author

PawelA commented Jan 18, 2021

Improved mask grid generation, but now it's much slower. I don't know how to make it faster with np or cv.

Signed-off-by: Paweł <pawel.anikiel@gmail.com>
yield check, results[i], inputs[i], tile, ratio, angle

def test_cpu_vs_cv_random():
batch_size = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned in the comment that now the tests are slower. I see that the batch size and number of iterations were doubled, which means the test has 4 times more work to do. If you want to reduce testing time I'd reduce the number of iterations (3 should be enough, I think)

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 did, but then i made them fast again (7f268fd), so I brought back the old parameters. I think the time taken by both tests is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

It takes 12s, looks good.

@JanuszL
Copy link
Contributor

JanuszL commented Jan 22, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2007899]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2007899]: BUILD PASSED

@JanuszL JanuszL merged commit 89afea0 into NVIDIA:master Jan 25, 2021
@JanuszL JanuszL mentioned this pull request May 19, 2021
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