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

Proposed affiliate package: Craterstats #5

Closed
wants to merge 4 commits into from

Conversation

ggmichael
Copy link

I propose to include a new python implementation of craterstats into PlanetaryPy

cheers,
Greg Michael

I propose to include a new python implementation of craterstats into PlanetaryPy

cheers,
Greg Michael
@rbeyer
Copy link
Member

rbeyer commented Mar 15, 2021

Thank you for proposing this package as an affiliated package!

In consensus discussion with the package author, @ggmichael, we decided not to begin formal review of the package quite yet, primarily because it has no tests (and would therefore review poorly), but we will keep this PR open in anticipation of tests being added. When the package author feels the package is ready to perform well against the PlanetaryPy review criteria, they will let us know via a comment on this PR.

@michaelaye
Copy link
Member

Hi @ggmichael , we would like to start the review process for your package. Could you suggest users of your package that could serve as reviewers?

@ggmichael
Copy link
Author

Hi Michael, I'm not aware of anyone who has used it for a publication yet. I think most users are still comfortable with the IDL VM version, even if they are glad to know this open source version exists. I'll ask a few people.

@michaelaye
Copy link
Member

This package has been reviewed for inclusion in the PlanetaryPy
affiliated package ecosystem by a member of the PlanetaryPy community
as well as myself, and I have synthesized the results of the review
here.

You can find out more about our review criteria in our
Review Guidelines.
For each of the review categories below we have listed the score and have
included some comments when the score is not green.

High level overview:

Overall Craterstats is functioning well and is a tested package that is useful for many planetary scientists.
We note a lack of documentation in the form of docstrings and inline comments that results in a "partial" grade in that category.
Additional code-quality issues are observed across the codebase from unused dependencies in the requirements.txt file to use of "eval" in Chronologyfn.py to inconsistent formatting and whitespacing throughout.
There are also utility functions that likely are satisfied by using more existing functionality from numpy/scipy.
Further improvements to the documentation of the package, particularly docstrings and inline comments, would make craterstats much more useful for users trying to use its Python API (although not an advertized use case) and for users trying to learn the crater counting math.

Functionality/Scope General
We consider craterstats to be a general package as multiple bodies are represented. It could be either used as a command line tool or, restricted, due to lack of docs but also not currently advertized, via its functional Python API, for interacting with crater production and chronology functions in the literature. So, it has applications beyond just one specific use case and can be applied to many domains involving impact processes.
Integration with PlanetaryPy ecosystem Good"
Without a core PlanetaryPy established yet, this package is establishing a lot of new functionality. It can however better use dependencies such as numpy to remove functions from the gm module for better maintenance.
Documentation Partial"
Craterstats has minimal documentation mainly contained in the main readme and only partially explains the command line tool, without documentation for using the package as a library with an API (although not advertized). Many functions are missing docstrings and inline comments to explain the intent of the code. The last line of the README points to a URL with tutorials which are good but could be more prominent in the beginning of the README, however the linked craterstats part of that demo only refers to the older IDL version of the tool. No governance documents are present in the repo, which is fine as the planetary ORG governance docs are applied when affiliated. The package maintainers should be strongly encouraged to adopt sphinx/readthedocs to produce documentation for the API and improved tutorials and examples.
Testing Partial"
Github CI tests indicate 72% code coverage at least, with additional coverage possibly from the tests that just run all the demo plots. Criteria specifies 90% for a grade of "good", so additional tests for missed code would be beneficial.
Development status Functional
We think the user circle could be improved with a bit more activity around documentation, tests, and reduced response time in the Github repo.
Python version compatibility 3.9"
The package itself only announces Python 3.8 as compatible version, however, we tested the install and demo run in Python 3.9 which completed without any issues, so we don't assume any issues here.

Summary/Decision: Everything looks great with a few caveats, but we're very happy to confirm that
this package is accepted as an affiliated package!

Please remember that if your package doesn't explicitly have a set
of governance documents, that's fine! The PlanetaryPy Project's
governance models (consensus seeking, code of conduct, etc.)
automatically apply to your project. If you want something different,
be sure to include it in your package.

If you have any follow-up questions or disagree with any of the comments above,
leave a comment and we can discuss it here. At any point in future you can
request a re-review of the package if you believe any of the scores should be
updated - contact the coordination committee, and we’ll do a new review.

Added correct values for the review section from Review text in PR thread.
@rbeyer
Copy link
Member

rbeyer commented May 17, 2024

Package author has chosen to withdraw this application for Affiliate Package status.

@rbeyer rbeyer closed this May 17, 2024
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.

3 participants