Skip to content
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

pVACview reviewer response #1062

Merged
merged 55 commits into from
May 8, 2024
Merged

Conversation

evelyn-schmidt
Copy link
Contributor

To add ease of use to pVACview, @huiming and I have created two new modules that can be used with input files from other neoantigen annotation tools. This includes a module for the popular tool NeoFox and a module called 'Custom' as it is meant to handle any neoantigen tool data. The new modules are implemented in the same server.R file as the base pVACview module but have separate ui.R.

NeoFox Module:
neofox_preview

Custom:
custom preview

@evelyn-schmidt evelyn-schmidt marked this pull request as ready for review February 6, 2024 23:56
@susannasiebert susannasiebert changed the base branch from master to staging February 8, 2024 18:56
Copy link
Contributor

@susannasiebert susannasiebert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me with some minor issues as noted in separate comments.

I assume we can't split out the server.R file into separate modules? It would be nice if it could be split out by pvacseq/neofox/custom input to organize things a bit more.

@@ -16,7 +16,7 @@ def define_parser():
def main(args_input = sys.argv[1:]):
parser = define_parser()
args = parser.parse_args(args_input)
arguments = ['{}'.format(args.r_path), "-e", "shiny::runApp('{}', port=3333)".format(args.pvacseq_dir)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason for this change? We need a consistent port in order for the deployed version to work.

@@ -1,5 +0,0 @@
__all__ = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to put this file back or the pvacview run command won't work correctly.

Copy link
Contributor

@susannasiebert susannasiebert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished review of the pVACview vignette in particular. I found a few typos and issues that lead to warnings when building the docs (title underlines too short; missing empty line after lists). I also expanded on a few explanations in the vignette where I felt more clarification could be useful.

docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
docs/pvacview/pvacseq_module/pvacseq_vignette.rst Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that there are additional R packages needed for the new modules. Can you please add them to the installation instruction?

@susannasiebert susannasiebert merged commit f8add9d into staging May 8, 2024
6 checks passed
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.

5 participants