Issue 430/all probabilities get predictions #433
Conversation
Lguyogiro
requested review from
desilinguist,
jbiggsets,
aoifecahill,
a user and
mulhod
There was a problem hiding this comment.
The actual algorithm for generating the probabilities is fine but the naming of some variables and arguments can be improved.
Also, can you please note in the PR description that this is will break backwards compatibility since we are now adding a header even in the single label case?
Also, we need to figure out why the coverage dropped.
|
@Lguyogiro what’s happening with this PR? Do you need help with the coverage issue? |
|
@desilinguist Sorry I got a little sidetracked with something else. I will add tests and hopefully have the coverage issues resolved by end of week. |
We need to do this since otherwise the travis builds time out.
…get-predictions Merge in MLP test fix
|
This is now passing and the coverage drop has been fixed. Please feel free to review 👍 |
| @@ -14,6 +14,7 @@ | |||
| import argparse | |||
| import logging | |||
| import os | |||
| import sys | |||
There was a problem hiding this comment.
Nitpick: It seems like this import is never used?
|
@Lguyogiro actually you still need to update the PR description per my original comment to say that this will break backwards compatibility. |
| all_labels: bool | ||
| A flag indicating whether to return the probabilities for all | ||
| labels in each row instead of just returning the probability of | ||
| `positive_label`. |
There was a problem hiding this comment.
This is a nitpick, but can we add "Defaults to False" here?
| probs_str = "\t".join([str(p) for p in probabilities]) | ||
| print("{}\t{}".format(id_, probs_str), file=outputfh) | ||
| else: | ||
| for i, pred in enumerate(preds): |
There was a problem hiding this comment.
Can we call this j or something else, since we're using i in the outer loop? Same comment below.
|
I've addressed @jbiggsets comments, and also added a unit test for the case of having multiple input files. |
|
I‘ll take a look at the other test you added on Monday. |
This PR implements the feature suggested in PR-430. Namely, it adds an option in
generate_predictionsto output the probability for all labels, instead of just the indicated positive label. It also updatesgenerate_predictionsto behave more like theLearner.predictmethod in that it prints out the predictions in tsv format, optionally to a file.NOTE: This is will break backwards compatibility since we are now adding a header even in the single label case