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

DM-10237: fix errors in blendedness calculation #80

Merged
merged 4 commits into from Apr 20, 2017
Merged

Conversation

TallJimbo
Copy link
Member

This addresses three problems in the blendedness calculation:

  • a simple transcription bug in what is now computeAbsExpectation (this is what originally spawened the ticket);
  • the integral in what is now computeAbsExpectation was over a nonnormalized probability;
  • we were truncating de-biased absolute values before summing them, which was introducing another bias we weren't correcting for.

I've also added some new unit tests.

@@ -284,6 +279,22 @@ BlendednessAlgorithm::BlendednessAlgorithm(Control const & ctrl, std::string co
}
}

float BlendednessAlgorithm::computeAbsExpectation(float data, float variance) {
float normalization = 0.5f*boost::math::erfc(-data/std::sqrt(2.0f*variance));
Copy link
Contributor

Choose a reason for hiding this comment

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

std::erfc available with C++11?

Copy link
Member Author

Choose a reason for hiding this comment

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

It hadn't occurred to me to check, but it looks like it is. I'll update all the code using the boost version in this file.

@@ -24,6 +24,8 @@
from contextlib import contextmanager
import unittest

import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover import from debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

From an older form of the test, yes.

@TallJimbo TallJimbo merged commit 56f2352 into master Apr 20, 2017
@ktlim ktlim deleted the tickets/DM-10237 branch August 25, 2018 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants