-
Notifications
You must be signed in to change notification settings - Fork 3
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
PR2 - Audio - To decibels HOST Tensor #150
PR2 - Audio - To decibels HOST Tensor #150
Conversation
Includes unittest setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snehaa8 Please address comments. Also PR2 shows all changes from PR1 (nonsilentregion) too. Hope this should go away as we merge PR1 in, and pull master into PR2 later.
if(minRatio == 0.0f) | ||
minRatio = std::nextafter(0.0f, 1.0f); | ||
|
||
const Rpp32f log10Factor = 0.3010299956639812; //1 / std::log(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize as #define in rpp_cpu_common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing that as #define even at the start of this file lead to output mismatches, thus left it as such
Rpp32u numThreads = handle.GetNumThreads(); | ||
|
||
// Calculate the intermediate values needed for DB conversion | ||
Rpp32f minRatio = std::pow(10, cutOffDB / multiplier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user gives multiplier as 0, is there a check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure added this check
{ | ||
*dstPtrCurrent = multiplier * std::log2(std::max(minRatio, (*srcPtrCurrent) * invReferenceMagnitude)); | ||
srcPtrCurrent++; | ||
dstPtrCurrent++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use post increment ++ at L84, and make loop single line removing braces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
*dstPtrTemp = multiplier * std::log2(std::max(minRatio, (*srcPtrTemp) * invReferenceMagnitude)); | ||
srcPtrTemp++; | ||
dstPtrTemp++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here on increments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
9f6b6d4
to
527ed18
Compare
if(minRatio == 0.0f) | ||
minRatio = std::nextafter(0.0f, 1.0f); | ||
|
||
const Rpp32f log10Factor = 0.3010299956639812; //1 / std::log(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snehaa8 Then instead of #define, lets just move the const Rpp32f as-is to rpp_cpu_common
* \param[in] multiplier factor by which the logarithm is multiplied | ||
* \param[in] referenceMagnitude Reference magnitude if not provided maximum value of input used as reference | ||
* \param[in] rppHandle HIP-handle for "_gpu" variants and Host-handle for "_host" variants | ||
* \return <tt> RppStatus enum</tt>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be before enum. Pls check non silent region and replicate
@@ -60,6 +60,25 @@ extern "C" { | |||
*/ | |||
RppStatus rppt_non_silent_region_detection_host(RppPtr_t srcPtr, RpptDescPtr srcDescPtr, Rpp32s *srcLengthTensor, Rpp32f *detectedIndexTensor, Rpp32f *detectionLengthTensor, Rpp32f cutOffDB, Rpp32s windowLength, Rpp32f referencePower, Rpp32s resetInterval, rppHandle_t rppHandle); | |||
|
|||
/*! \brief To Decibels augmentation HOST | |||
* \details To Decibels augmentation that converts magnitude values to decibel values | |||
* \param[in] srcPtr source tensor memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets specify HOST as per the new doxygen style
@@ -173,6 +195,29 @@ int main(int argc, char **argv) | |||
maxWallTime = std::max(maxWallTime, wallTime); | |||
minWallTime = std::min(minWallTime, wallTime); | |||
avgWallTime += wallTime; | |||
|
|||
// QA mode - verify outputs with golden outputs. Below code doesn’t run for performance tests | |||
if (testType == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we increase to 8 reference outputs for to_decibels? QA has always been 3, non_silent_region has 3 too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snehaa8 Could you remove those reference outputs not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done removed extra ones, it was 8 initially and Sampath made this change in NSR at a later point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snehaa8 Please issue a separate PR for changes mentioned.
However seeing on 54 machine, even for image tensor kernels. |
No description provided.