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

Normalize GPU - pImpl + Bessel's corrections #1981

Merged
merged 5 commits into from
May 27, 2020

Conversation

mzient
Copy link
Contributor

@mzient mzient commented May 25, 2020

Why we need this PR?

Pick one, remove the rest

  • It adds the following features:
    • pImpl front-end for Normalize
    • Bessel's corrections in StdDev / InvStdDev

What happened in this PR?

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

  • What solution was applied:
    • pImpl + template implementation in source file + explicit instantiation + extern templates
    • change postprocessing for stddev to subtract degrees of freedom from reduction factor
  • Affected modules and functionalities:
    • Normalize GPU
    • StdDev GPU
  • Key points relevant for the review:
    • Bessel's correction - check parameter forwarding (easy to miss because of default values)
  • Validation and testing:
    • Unit tests modified to use pImpl for some cases
  • Documentation (including examples):
    • Doxygen

JIRA TASK: DALI-1267

@mzient mzient requested a review from a team May 25, 2020 19:02
@mzient mzient mentioned this pull request May 26, 2020
Add Bessel's correction for standard deviation.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@mzient mzient changed the title [DoNotMerge] Normalize GPU - pImpl + Bessel's corrections Normalize GPU - pImpl + Bessel's corrections May 27, 2020
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1349891]: BUILD STARTED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@@ -21,6 +21,7 @@
#include "dali/core/convert.h"
#include "dali/core/format.h"
#include "dali/core/small_vector.h"
#include "dali/core/tensor_shape_print.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it needed here?

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 must re-check... I'm formatting tensor shape in some error messages and I think the error originated in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked - it is indeed necessary.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1349891]: BUILD PASSED

* (typically reciprocal of standard deviation or 1/(max-min)).
* The normalization follows the formula:
* ```
* out[data_idx] = (in[data_idx] - base[param_idx]) * scale[param_idx] * global_scale + shift
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can write how param_idx is calculated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it is described in L54. Still small info hear would help.

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 explained it in lines 51-57.

@mzient
Copy link
Contributor Author

mzient commented May 27, 2020

!build

* ```
* Where `data_idx` is a position in the data tensor (in, out) and `param_idx` is a position
* in the base and scale tensors (see below for details). The two additional constants,
* `global_scale` and `shift` can be used to adjust the result to the ynamic range and resolution
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
* `global_scale` and `shift` can be used to adjust the result to the ynamic range and resolution
* `global_scale` and `shift` can be used to adjust the result to the dynamic range and resolution

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1350221]: BUILD STARTED

@mzient mzient force-pushed the NormalizeGPU_v2 branch 2 times, most recently from c9801aa to af66a83 Compare May 27, 2020 15:23
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1350287]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1350287]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1350287]: BUILD PASSED

@mzient mzient merged commit 9125deb into NVIDIA:master May 27, 2020
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

4 participants