Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

austereantelope
Copy link
Contributor

@austereantelope austereantelope commented Jan 24, 2022

Patch description
Hi,

The test test_sampled_equals_unsampled_when_biased_against_non_sampled_positions in sampled_softmax_loss_test.py has an assertion bound (assert (abs(pct_error) < 0.001)) that is too loose. This means potential bug in the code could still pass the original test.

To quantify this I conducted some experiments where I generated multiple mutations of the source code under test and ran each mutant and the original code 100 times to build a distribution of their outputs. Each mutant is generated using simple mutation operators (e.g. > can become < ) on source code covered by the test. I used KS-test to find mutants that produced a different distribution from the original and use those mutants as a proxy for bugs that could be introduced. In the graph below I show the distribution of both the original code and also the mutants with a different distribution.

Here we see that the bound of 0.001 is too loose since the original distribution (in orange) is less than 0.001. Furthermore in this graph we can observe that there are many mutants (proxy for bugs) that are below the bound as well and that is undesirable since the test should aim to catch potential bugs in code. I quantify the "bug detection" of this assertion by varying the bound in a trade-off graph below.

In this graph, I plot the mutant catch rate (ratio of mutant outputs that fail the test) and the original pass rate (ratio of original output that pass the test). The original bound of 0.001 (red dotted line) has a catch rate of 0.51.

To improve this test, I propose to tighten the bound to 0.0003 (the blue dotted line). The new bound has a catch rate of 0.66 (+0.15 increase compare to original) while still has >99 % pass rate (test is not flaky, I ran the updated test 500 times and observed 100 % pass rate). I think this is a good balance between improving the bug-detection ability of the test while keep the flakiness of the test low.

Do you guys think this makes sense? Please let me know if this looks good or if you have any other suggestions or questions.

My Environment:

python=3.7.11
pytorch=1.10.0

my allennlp Experiment SHA:
2cdb8742c8c8c3c38ace4bdfadbdc750a1aa2475

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.
  • codecov/patch reports high test coverage (at least 90%).
    You can find this under the "Actions" tab of the pull request once the other checks have finished.

@AkshitaB
Copy link
Contributor

AkshitaB commented Feb 1, 2022

@austereantelope This is great! Thanks for being so thorough!

@AkshitaB AkshitaB enabled auto-merge (squash) February 1, 2022 08:16
@AkshitaB AkshitaB merged commit 3c2299a into allenai:main Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants