Skip to content

Fix false positives for ignored mismatches.#8

Merged
jesseplusplus merged 1 commit intogithub:masterfrom
calavera:matched_false_positive
Dec 20, 2014
Merged

Fix false positives for ignored mismatches.#8
jesseplusplus merged 1 commit intogithub:masterfrom
calavera:matched_false_positive

Conversation

@calavera
Copy link
Contributor

Result#matched? returns true when it was actually an ignored mismatch.

In the publishing example in the README, for instance:

class MyExperiment
  def publish(result)
    # Store the timing for the control value,
    $statsd.timing "science.#{name}.control", result.control.duration
    # for the candidate (only the first, see "Breaking the rules" below,
    $statsd.timing "science.#{name}.candidate", result.candidates.first.duration

    # and counts for match/ignore/mismatch:
    if result.matched?
      $statsd.increment "science.#{name}.matched"
    elsif result.ignored?
      $statsd.increment "science.#{name}.ignored"
    else
      $statsd.increment "science.#{name}.mismatched"
      # Finally, store mismatches in redis so they can be retrieved and examined
      # later on, for debugging and research.
      store_mismatch_data(result)
    end
  end

Given this experiment:

science "test" do |e|
  e.ignore { true }
  e.use { true }
  e.try { false }
end

The publish method will increment the counter for science.test.matched rather than incrementing science.test.ignored.

@calavera
Copy link
Contributor Author

Btw, I'm aware I can fix this in my code by switching the order in which I check Result#matched? and Result#ignored?, but I still think it's unexpected to return true when the observations didn't really match.

@calavera
Copy link
Contributor Author

@jesseplusplus, @jbarnette I'd really appreciate some comments about this behavior.

@zerowidth zerowidth self-assigned this Dec 15, 2014
@jesseplusplus
Copy link

Definitely a bug.

👍 nice catch and fix

jesseplusplus pushed a commit that referenced this pull request Dec 20, 2014
Fix false positives for ignored mismatches.
@jesseplusplus jesseplusplus merged commit 00e3218 into github:master Dec 20, 2014
@zerowidth
Copy link
Contributor

I vaguely recall doing this on purpose, but this makes more sense. 👍 and thanks for merging @jesseplusplus

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.

3 participants