Skip to content

Fix CropMirrorNormalize crop_pos_x/y argument for the CPU#853

Merged
JanuszL merged 1 commit intoNVIDIA:masterfrom
JanuszL:fix_cpu_cmn
May 10, 2019
Merged

Fix CropMirrorNormalize crop_pos_x/y argument for the CPU#853
JanuszL merged 1 commit intoNVIDIA:masterfrom
JanuszL:fix_cpu_cmn

Conversation

@JanuszL
Copy link
Contributor

@JanuszL JanuszL commented May 7, 2019

  • makes CropMirrorNormalize take into account crop_pos_x and
    crop_pos_y arguments

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [727946]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [727946]: BUILD FAILED

Copy link
Contributor

Choose a reason for hiding this comment

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

The distribution is skewed. Should be:

Suggested change
int crop_y = crop_y_image_coord * (H - crop_h_);
int crop_y = std::floor(crop_y_image_coord * (H - crop_h_) + 0.5f);

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 do (for GPU and crop itself as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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
int crop_x = crop_x_image_coord * (W - crop_w_);
int crop_x = std::floor(crop_x_image_coord * (W - crop_w_) + 0.5f);

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 do (for GPU and crop itself as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [729062]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [729062]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [729130]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [729130]: BUILD PASSED

Copy link
Contributor

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/numeric/math/round

Suggested change
const int crop_y = std::floor(0.5f * (rsz_h - crop_h) + 0.5f);
const int crop_y = std::round(0.5f * (rsz_h - crop_h));

same applies to other occurrences

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [730546]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [730546]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [730648]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [730648]: BUILD FAILED

- makes CropMirrorNormalize take into account crop_pos_x and
  crop_pos_y arguments

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [730775]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [730775]: BUILD PASSED

@JanuszL JanuszL merged commit b8ef571 into NVIDIA:master May 10, 2019
@JanuszL JanuszL deleted the fix_cpu_cmn branch May 10, 2019 08:06
kychennv pushed a commit to kychennv/DALI that referenced this pull request May 14, 2019
- makes CropMirrorNormalize take into account crop_pos_x and
  crop_pos_y arguments

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Signed-off-by: kychennv <kychen@nvidia.com>
haoxintong pushed a commit to haoxintong/DALI that referenced this pull request Jul 16, 2019
- makes CropMirrorNormalize take into account crop_pos_x and
  crop_pos_y arguments

Signed-off-by: Janusz Lisiecki <jlisiecki@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.

4 participants