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.

Sign up for GitHub

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

Jump to bottom

PspA figure implementation #93

Closed
wants to merge 2 commits into from
Closed

PspA figure implementation #93

wants to merge 2 commits into from

Conversation

falquaddoomi
Copy link
Contributor

@falquaddoomi falquaddoomi commented Oct 9, 2024 •
edited by jananiravi
Loading

Description

This PR contains a very work-in-progress implementation of figure 4 from the PspA paper, https://journals.asm.org/doi/epub/10.1128/msystems.00847-23. The PR
is intended as a starting point, so you don't have to go and find the original code from the papers, but it definitely needs a lot of work! See issue #90 for more details.

Closes #90.

What kind of change(s) are included?

  • Feature (adds or updates new capabilities)
  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (these changes would cause existing functionality to not work as expected).

Checklist

Please ensure that all boxes are checked before indicating that this pull request is ready for review.

  • I have read and followed the CONTRIBUTING.md guidelines.
  • I have searched for existing content to ensure this is not a duplicate.
  • I have performed a self-review of these additions (including spelling, grammar, and related).
  • I have added comments to my code to help provide understanding.
  • I have added a test which covers the code changes found within this PR.
  • I have deleted all non-relevant text in this pull request template.
  • Reviewer assignment: Tag a relevant team member to review and approve the changes.

@falquaddoomi falquaddoomi mentioned this pull request Oct 9, 2024
Open
@jananiravi jananiravi added enhancement New feature or request dataviz Data visualization labels Oct 20, 2024
jananiravi
jananiravi requested changes Oct 30, 2024
View reviewed changes
Copy link
Member

@jananiravi jananiravi left a comment

Choose a reason for hiding this comment

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

I've moved things around a bit to at least clean up towards a working function. It's clearly not quite there yet. I hope this helps as a start.
I need a little help, @the-mayer @epbrenner -- thanks!

R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Outdated Show resolved Hide resolved
R/pspViz.R Show resolved Hide resolved
@falquaddoomi
Copy link
Contributor Author

falquaddoomi commented Dec 10, 2024 •
edited
Loading

Hey @jananiravi, thanks for the review. Would it be alright if I committed your suggestions? They all seem reasonable to me. (Well, since molevol_scripts is directly sourced by the Shiny app, not imported as a package, I'll likely keep the library() calls and other source() statements that refer to the rest of the code, just so things work, but I'll keep removing them in mind for when we go back to making all these figure-generating functions package methods.)

Also, I was planning to create a new PR in https://github.com/JRaviLab/molevol_scripts with this PR's contents. I had originally created PRs here for each figure so our contributors could work on it, but since no one took it on and it's needed for the rebuttal I was thinking it'd be faster to get it merged into molevol_scripts first. When we're ready, I can port it back over to the package.

If that's all OK with you I'll start working on that, and I'll do the same for DciA and InlP.

@JRaviLab JRaviLab deleted a comment from notion-workspace bot Dec 10, 2024
@jananiravi
Copy link
Member

Sounds good, Faisal. If you have any additional questions, please feel free to tag @epbrenner and me, and we will get back to you ASAP. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jananiravi jananiravi jananiravi requested changes

@the-mayer the-mayer Awaiting requested review from the-mayer

@epbrenner epbrenner Awaiting requested review from epbrenner

Assignees

@the-mayer the-mayer

@epbrenner epbrenner

Labels
dataviz Data visualization enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT] Fix, Implement PspA Figure
4 participants
@falquaddoomi @jananiravi @the-mayer @epbrenner