search

pyOpenSci / software-submission

Submit your package for review by pyOpenSci here! If you have questions please post them here: https://pyopensci.discourse.group/
94 stars 35 forks source link

plot_phylo #160

Open KatyBrown opened 9 months ago

KatyBrown commented 9 months ago

Submitting Author: Katy Brown (@KatyBrown) All current maintainers: Katy Brown (@KatyBrown) Package Name: plot_phylo One-Line Description of Package: A Python package to plot a phylogenetic tree on an existing Matplotlib axis. Repository Link: https://github.com/KatyBrown/plot_phylo Version submitted: 1.9.0 EIC: @isabelizimm Editor: @ctb Reviewer 1: @ammaraziz Reviewer 2: @jkanche Archive: TBD JOSS DOI: TBD Version accepted: TBD Date accepted (month/day/year): TBD

Code of Conduct & Commitment to Maintain Package

Description

plot_phylo is a Python package which allows the user to plot phylogenetic trees onto an existing matplotlib axis. This means annotations can be added using matplotlib functionality and figures including phylogenies can be included in automated figure generation.

Scope

Domain Specific & Community Partnerships 

- [ ] Geospatial
- [ ] Education
- [ ] Pangeo

Community Partnerships

If your package is associated with an existing community please check below:

[^1]: Please fill out a pre-submission inquiry before submitting a data visualization package.

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication Options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] The package contains a `paper.md` matching [JOSS's requirements][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: *Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Confirm each of the following by checking the box.

Please fill out our survey

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

isabelizimm commented 9 months ago

Hello there, and welcome to the pyOpenSci community-- we are so happy you are here! Just wanted to let you know we have seen your submission and will get back to you with some initial checks soon. 👾

isabelizimm commented 9 months ago

Editor in Chief checks

Hi there! Thank you for submitting your package for pyOpenSci review. Below are the basic checks that your package needs to pass to begin our review. If some of these are missing, we will ask you to work on them before the review process begins.

Please check our Python packaging guide for more information on the elements below.

Editor comments

Thank you again for submitting plot_phylo to pyOpenSci! I've gone through your package, and you've got a great start!

Blockers to beginning review process:

Some smaller nits:

If you have any questions or need any support with these tasks, pyOpenSci community would love to help out! The best places to get in touch are via discourse or Slack--let me know if you need an invite. And, of course, you can also reply here (but then you will be restricted to just my opinions 😂)!

isabelizimm commented 8 months ago

Following up, @ctb will the editor leading the review for plot_phylo once the above items have been addressed! Titus also has a few things to finish up before starting this role, so I'll check back here in about 2 weeks to check in if both parties are ready to kick off this review 😄

isabelizimm commented 8 months ago

Hello all! Checking in here. @KatyBrown are you able to give an update on the status of a documentation website? We won't be able to start a review without that put up. If you need any support, please let us know!

KatyBrown commented 8 months ago

Hi @isabelizimm , sorry, I got caught in other projects but I'm working on moving the documentation the readthedocs - it should be ready in another couple of days. Sorry to be slow! Thanks for checking in.

isabelizimm commented 8 months ago

No worries at all--thank you for the update! 🌻 If you can give us a heads up in this issue once those docs are up, then we can get started on the review.

ctb commented 7 months ago

I am alive! Just let me know when to start finding reviewers :)

KatyBrown commented 7 months ago

Hi @isabelizimm, very sorry for the long delay, I've now made the changes you asked for. The current version of the tool is now v0.1.9 - I've edited this in my original post.

Specifically I have:

Please let me know if there's anything else I need to do, otherwise I think I'm now ready to proceed to the next step :)

Thank you!

isabelizimm commented 7 months ago

No worries-- with those all updated, I can hand everything over to @ctb to look for reviewers!

ctb commented 6 months ago

Apologies for delay - started beating the bushes for some reviewers. In the meantime, filed a few minor issues myself ;). The figures are lovely!!

KatyBrown commented 6 months ago

@ctb thanks very much! I think I've addressed the issues you raised.

ctb commented 5 months ago

apologies for my long delay - have one reviewer locked in, reach out to some more today.

ctb commented 4 months ago

Editor response to review:

Editor comments

:wave: Hi @ammaraziz and @jkanche! Thank you for volunteering to review for pyOpenSci! Sorry it took me so long to post this, I blame ...umm, summer! Yeah, it's summer time! But seriously, thank you so much!!

Please fill out our pre-review survey

Before beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs.

The following resources will help you complete your review:

  1. Here is the reviewers guide. This guide contains all of the steps and information needed to complete your review.
  2. Here is the review template that you will need to fill out and submit here as a comment, once your review is complete.

Please get in touch with any questions or concerns! Your review is due: July 22nd, 2024!

Reviewers: @ammaraziz and @jkanche Due date: July 22nd, 2024

jkanche commented 3 months ago

Apologies for the delay, I plan to finish my review this week.

ctb commented 3 months ago

Apologies for the delay, I plan to finish my review this week.

thanks for checking in! @ammaraziz, need anything from me?

jkanche commented 3 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

Review Comments

I cloned the repo and installed it in edit mode:

⋊> ~/P/p/t/plot_phylo on main ◦ pytest --cov=plot_phylo                                   (reviews) 16:09:15
=========================================== test session starts ============================================
platform darwin -- Python 3.11.9, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/kancherj/Projects/public/trash/plot_phylo
configfile: pyproject.toml
plugins: cov-5.0.0
collected 0 items / 3 errors
/opt/homebrew/lib/python3.11/site-packages/coverage/control.py:888: CoverageWarning: No data was collected. (no-data-collected)
  self._warn("No data was collected.", slug="no-data-collected")

================================================== ERRORS ==================================================
______________________________ ERROR collecting tests/test_get_boxes_data.py _______________________________
ImportError while importing test module '/Users/kancherj/Projects/public/trash/plot_phylo/tests/test_get_boxes_data.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/homebrew/Cellar/python@3.11/3.11.9/Frameworks/Python.framework/Versions/3.11/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_get_boxes_data.py:2: in <module>
    import matplotlib.pyplot as plt
E   ModuleNotFoundError: No module named 'matplotlib'
________________________________ ERROR collecting tests/test_plot_phylo.py _________________________________
ImportError while importing test module '/Users/kancherj/Projects/public/trash/plot_phylo/tests/test_plot_phylo.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/homebrew/Cellar/python@3.11/3.11.9/Frameworks/Python.framework/Versions/3.11/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_plot_phylo.py:2: in <module>
    import matplotlib.pyplot as plt
E   ModuleNotFoundError: No module named 'matplotlib'
______________________________ ERROR collecting tests/test_plot_phylo_data.py ______________________________
ImportError while importing test module '/Users/kancherj/Projects/public/trash/plot_phylo/tests/test_plot_phylo_data.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/homebrew/Cellar/python@3.11/3.11.9/Frameworks/Python.framework/Versions/3.11/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_plot_phylo_data.py:2: in <module>
    import plot_phylo
E   ModuleNotFoundError: No module named 'plot_phylo'

---------- coverage: platform darwin, python 3.11.9-final-0 ----------
Name                       Stmts   Miss  Cover   Missing
--------------------------------------------------------
plot_phylo/plot_phylo.py     144    144     0%   2-586
--------------------------------------------------------
TOTAL                        144    144     0%

========================================= short test summary info ==========================================
ERROR tests/test_get_boxes_data.py
ERROR tests/test_plot_phylo.py
ERROR tests/test_plot_phylo_data.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 3 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Overall, the package provides an intuitive and easy way to integrate phylogeny visualizations with matplotlib based workflows. Let me know if you have any questions.

ammaraziz commented 3 months ago

Hi @ctb and @KatyBrown, please accept my apologies. I've been stretched thin over the last few months.

I will make time for reviewing this week if that is acceptable?

ctb commented 3 months ago

Hi @ctb and @KatyBrown, please accept my apologies. I've been stretched thin over the last few months.

as have we all! no worries and I really appreciate your willingness to review!

I will make time for reviewing this week if that is acceptable?

please and thank you!

ammaraziz commented 3 months ago

Package Review

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing:

Review Comments

Great package, works nicely and the provided example is a great start. The power of matplotlib means it's possible to make great plots.

  1. Similar to the previous review, when testing the package I get errors. However, the documentation states:

Run unit tests in the root directoy as pytest --cov=plot_phylo and ensure that they pass and that coverage is 100%.

But I get an error:

pytest: error: unrecognized arguments: --cov=plot_phylo

Testing works if you run just pytest in the root dir:

============================== test session starts ===============================
platform darwin -- Python 3.9.19, pytest-8.3.2, pluggy-1.5.0
rootdir: ~Desktop/plot_phylo
configfile: pyproject.toml
collected 138 items

tests/test_plot_phylo.py ................................................. [ 35%]
.......................................................................... [ 89%]
...............                                                            [100%]

================================ warnings summary ================================
tests/test_plot_phylo.py:23
  /Users/aaziz/Desktop/plot_phylo/tests/test_plot_phylo.py:23: MatplotlibDeprecationWarning: Auto-close()ing of figures upon backend switching is deprecated since 3.8 and will be removed in 3.10.  To suppress this warning, explicitly call plt.close('all') first.
    matplotlib.use('Agg')

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================== 138 passed, 1 warning in 62.46s (0:01:02) ====================
  1. Consider adding some basic usage docs to the readme to get people started.
  2. Provide example of how to color internal nodes and branches.
  3. Consider using github actions for testing and publishing.
  4. Add a section on how to cite your work, even if it's just the github repo, for example: 
    # Cite
KatyBrown - plot_phylo https://github.com/KatyBrown/plot_phylo

Good job on the great package.

ctb commented 3 months ago

thanks @ammaraziz and @jkanche!!

@KatyBrown please take a detailed look at the above, and I will do the same!

ctb commented 3 months ago

@jkanche https://pyscaffold.org/en/stable/ looks fascinating!

ctb commented 3 months ago

@jkanche and @ammaraziz, thank you so much for your reviews!!

@KatyBrown, I've extracted the unchecked bits of the reviews below -- please see bottom for my thoughts!

pyopensci -- summary of reviews by CTB

Package review by @jkanche:

Documentation

The package includes all the following forms of documentation:

The README should include, from top to bottom:

Usability

In general please consider whether:

Functionality

Review Comments

I cloned the repo and installed it in edit mode:

(errors elided)

CTB comment: you could put these in the issue tracker, if you didn't want to implement them now :)

Overall, the package provides an intuitive and easy way to integrate phylogeny visualizations with matplotlib based workflows. Let me know if you have any questions.

Package Review by @ammaraziz

Documentation

The README should include, from top to bottom:

Functionality

Review Comments

Great package, works nicely and the provided example is a great start. The power of matplotlib means it's possible to make great plots.

  1. Similar to the previous review, when testing the package I get errors. However, the documentation states:

Run unit tests in the root directoy as pytest --cov=plot_phylo and ensure that they pass and that coverage is 100%.

But I get an error:

pytest: error: unrecognized arguments: --cov=plot_phylo

Testing works if you run just pytest in the root dir:

(elided)

  1. Consider adding some basic usage docs to the readme to get people started.
  2. Provide example of how to color internal nodes and branches.
  3. Consider using github actions for testing and publishing.
  4. Add a section on how to cite your work, even if it's just the github repo, for example: 
    # Cite
KatyBrown - plot_phylo https://github.com/KatyBrown/plot_phylo

Good job on the great package.

Editor comments from CTB

The three major suggestions from the reviews that stand out are -

The rest all seem like fairly quick add-this/adjust-that changes.

Please let me know if you have thoughts or questions!

KatyBrown commented 2 months ago

Hi @ctb @jkanche @ammaraziz thanks so much for looking at this and sorry for my slow response. I will look at your comments over the next few days.

One quick thing, I do have CI testing which runs my test suite - the workflow is here https://github.com/KatyBrown/plot_phylo/actions/workflows/main.yml. I'm not sure if they are not showing up somehow.

Thanks again and I'll address everything else asap.

KatyBrown commented 1 month ago

Hi everyone, apologies for my extremely slow response to this. I really appreciate all the feedback on my package. I've addressed specific comments below. All changes are present in release 0.1.11

KatyBrown commented 1 month ago

@jkanche

Review Comments

  • Cannot run tests following the instructions

Sorry about that, I’ve added new instructions to the manual - which hopefully now work.

  • Add a short description and link to docs for the repository metadata. Helps users quickly understand the scope and link to the documentation (right side bar on the repository page):

I have now added these to the main page.

  • Publishing seems to be manual right now and don't understand how that is setup? I usually use pyscaffold and it makes it easier for some routine package maintenance tasks.

I’ve now added publishing automatically to pip as a GitHub workflow - I’ll also look into this package as it sounds useful!

  • code quality (linting) is not included in the CI/CD tests.

It is now included

  • As I was reviewing the package, I thought a couple of features may be super helpful:

    • Highlight paths/features of interest
    • Ability to choose or specify branch widths

CTB comment: you could put these in the issue tracker, if you didn't want to implement them now :)

I’ve added these two as enhancements in the issue tracker - as I have a few other enhancements I’d like to make at the same time and I don't want to delay this even further.

Overall, the package provides an intuitive and easy way to integrate phylogeny visualizations with matplotlib based workflows. Let me know if you have any questions.

Thank you very much!

KatyBrown commented 1 month ago

@ammaraziz

Great package, works nicely and the provided example is a great start. The power of matplotlib means it's possible to make great plots.

Thank you! I appreciate it and I’m glad it works!

  1. Similar to the previous review, when testing the package I get errors. However, the documentation states:

Run unit tests in the root directoy as pytest --cov=plot_phylo and ensure that they pass and that coverage is 100%.

But I get an error:

pytest: error: unrecognized arguments: --cov=plot_phylo

Testing works if you run just pytest in the root dir:

Apologies for this, I’ve updated the manual - with instructions which hopefully now work.

  1. Consider adding some basic usage docs to the readme to get people started.

I’ve added a QuickStart guide to the README.md along with an additional link to the full documentation, so it’s easier to find.

  1. Provide example of how to color internal nodes and branches.

I already have this example here but I’ve tried to make the manual easier to find from the README.

  1. Consider using github actions for testing and publishing.

I was already using GitHub actions for CI testing here, I’ve added details of this to the README.md and CONTRIBUTING.md files so it is easier to find. I’ve also now added publishing automatically to pip as a GitHub workflow

  1. Add a section on how to cite your work, even if it's just the github repo, for example:

Thank you - I’ve added this now to the README

Good job on the great package.

Thank you very much!

KatyBrown commented 1 month ago

@ctb

The three major suggestions from the reviews that stand out are -

  • a few more usage docs are suggested/would be welcome!

I think it was not very clear where to find my documentation, I’ve added additional links. If there is still not enough detail please let me know.

  • test the instructions for running the tests, and/or improve the documentation;

I’ve updated the manual - with instructions which hopefully now work.

  • suggest using GitHub CI - lmk if you need some pointers to quickstarts! I have some projects that do basically what you need, I think. Note that putting CI in place will help with the test running & documentation, above, too!

Thank you. I was already using GitHub actions for CI testing here, I’ve added details of this to the README.md and CONTRIBUTING.md files so it is easier to find. I’ve also now added publishing automatically to pip as a GitHub workflow

The rest all seem like fairly quick add-this/adjust-that changes.

Please let me know if you have thoughts or questions!

Thank you!

@ctb I’ve also done the following to address additional things on the checklist:

KatyBrown commented 1 month ago

Thanks very much everyone for the feedback and please let me know if there's any more I can do!