Conversation
…odel artifacts; modified argument parser and pvacseq run.py file to run the predictor as part of pvacseq; added test files and script.
jyao36
changed the title
There was a problem hiding this comment.
This looks pretty good. I added some comments for improvements that could be made.
I would also like to see the addition of a standalone pvacseq add_ml_predictions command that someone could run standalone to add ml_predictions to the output from their pipeline if the ml option was not enabled in the original run. Have a look at pvactools/tools/pvacseq/mark_genes_of_interest.py and its test tests/test_pvacseq_mark_genes_of_interest.py to see how to do so. This command addition also requires an update to pvactools/tools/pvacseq/main.py and pvactools/tools/pvacseq/__init__.py to add this command.
Additionally, this new command should then be documented in docs/pvacseq/optional_downstream_analysis_tools.rst. Most of the documentation can be auto-generated from the command line docs by using the program-output tool but this would also be the spot to put any additional documentation you want to have around this feature.
susannasiebert
left a comment
susannasiebert
left a comment
There was a problem hiding this comment.
Left a couple of code suggestions to convert named required parameters to positional parameters in your parser.
Co-authored-by: Susanna Kiwala <susanna.kiwala@wustl.edu>
Co-authored-by: Susanna Kiwala <susanna.kiwala@wustl.edu>
Co-authored-by: Susanna Kiwala <susanna.kiwala@wustl.edu>
…or; updated argument format for add_ml_predictions.py
susannasiebert
left a comment
susannasiebert
left a comment
There was a problem hiding this comment.
I made some copy editing suggestions. Two thoughts I had while reviewing this:
- It would be great to have some additional exploration of different candidates in ML section of the pVACview vignette. I wrote down some examples in a comment in that part of the text.
- Have you and Malachi discussed whether it would be worthwhile to make the reject threshold configurable? It seems a bit weird to me to hardcode the reject threshold but make the accept threshold configurable.
| - Directory containing image files for pVACview. Not generated when running with presentation and immunogenicity algorithms only. | ||
| * - ``ml_predict/<sample_name>_predict_pvacview.tsv`` (optional) | ||
| - ML-based neoantigen evaluation predictions file. Generated when both MHC Class I and Class II predictions are run and the ``--run-ml-predictions`` flag is set. | ||
| - Directory containing image files for pVACview. Not generated when running only with presentation and immunogenicity algorithms only. |
There was a problem hiding this comment.
| - Directory containing image files for pVACview. Not generated when running only with presentation and immunogenicity algorithms only. |
This should fix the issue with this table not rendering.
… columns and fill with NaN; Updated ML file name, argument names; Updated documentation
susannasiebert
left a comment
susannasiebert
left a comment
There was a problem hiding this comment.
This looks good. Just two minor change requests.
| parser.add_argument( | ||
| "output_dir", | ||
| help="Directory where the ML prediction TSV files should be written." | ||
| nargs="?", |
There was a problem hiding this comment.
Instead of using nargs="?" and a positional parameter for optional parameters, we've been using the -- convention (like for the --ml-threshold-accept and --ml-threshold-predict). If you rename the parameter to --output-dir it will automatically be optional. It should also be moved behind all of the positional parameters, i.e between sample_name and --ml-threshold-accept).
Co-authored-by: Susanna Kiwala <susanna.kiwala@wustl.edu>
2 participants
…odel artifacts; modified argument parser and pvacseq run.py file to run the predictor as part of pvacseq; added test files and script.