-
Notifications
You must be signed in to change notification settings - Fork 72
ML predictor integration into pvacseq #1333
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
base: 7.0.0
Are you sure you want to change the base?
Conversation
…odel artifacts; modified argument parser and pvacseq run.py file to run the predictor as part of pvacseq; added test files and script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of code suggestions to convert named required parameters to positional parameters in your parser.
Co-authored-by: Susanna Kiwala <[email protected]>
Co-authored-by: Susanna Kiwala <[email protected]>
Co-authored-by: Susanna Kiwala <[email protected]>
…or; updated argument format for add_ml_predictions.py
...ml_predictor/ml_predictor_test_output/HCC1395.MHC_I.all_epitopes.aggregated.ML_predicted.tsv
Show resolved
Hide resolved
susannasiebert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
susannasiebert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
susannasiebert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| * - ``ui.R``, ``app.R``, ``server.R``, ``styling.R``, ``anchor_and_helper_functions.R`` | ||
| - pVACview R Shiny application files. Not generated when running only with presentation and immunogenicity algorithms. | ||
| * - ``www`` (directory) | ||
| - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table doesn't seem to be showing up on the webpage
docs/pvacseq/output_files.rst
Outdated
| - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - 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
…odel artifacts; modified argument parser and pvacseq run.py file to run the predictor as part of pvacseq; added test files and script.