PetroFit

[robelgeda]

Submitting Author: Robel Geda (@robelgeda)
All current maintainers: (@robelgeda, @crawfordsm)
Package Name: PetroFit
One-Line Description of Package:The PetroFit Project is an open-source effort to develop end-to-end tools for making accurate photometric measurements, estimating morphological properties, and fitting 2D models to galaxy images.
Repository Link: https://github.com/PetroFit/petrofit
Version submitted: v0.5.0 (will be updated to v1.0.0 after review)
EIC: @isabelizimm
Editor: @dhomeier
Reviewer 1: @nden
Reviewer 2: @kyleaoman
Archive: TBD
JOSS DOI: TBD
Version accepted: TBD
Date accepted (month/day/year): TBD

---

Code of Conduct & Commitment to Maintain Package

• I agree to abide by pyOpenSci's Code of Conduct during the review process and in maintaining my package after should it be accepted.
• I have read and will commit to package maintenance after the review as per the pyOpenSci Policies Guidelines.

Description

The goal of the PetroFit Python package, which is based on Astropy and Photutils, is to provide specialized tools for the astronomical community. It is designed for calculating Petrosian properties, such as radii and concentration indices of galaxies, as well as fitting galaxy light profiles. In particular, PetroFit includes tools for performing accurate photometry, segmentations, Petrosian profiling, and Sérsic fitting.

Scope

• Please indicate which category or categories.
  Check out our package scope page to learn more about our
  scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):

  • Data retrieval
  • Data extraction
  • Data processing/munging
  • Data deposition
  • Data validation and testing
  • Data visualization¹
  • Workflow automation
  • Citation management and bibliometrics
  • Scientific software wrappers
  • Database interoperability

Domain Specific

- [ ] Geospatial
- [ ] Education

Community Partnerships

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

Domain Specific

• Geospatial
• Education

Community Partnerships

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

• Astropy: Link coming soon to standards
• Pangeo: My package adheres to the Pangeo standards listed in the pyOpenSci peer review guidebook

• For all submissions, explain how the and why the package falls under the categories you indicated above. In your explanation, please address the following points (briefly, 1-2 sentences for each):

  • Who is the target audience and what are scientific applications of this package?

  • Are there other Python packages that accomplish the same thing? If so, how does yours differ?

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted:

Technical checks

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

• does not violate the Terms of Service of any service it interacts with.
• uses an OSI approved license.
• contains a README with instructions for installing the development version.
• includes documentation with examples for all functions.
• contains a tutorial with examples of its essential functions and uses.
• has a test suite.
• has continuous integration setup, such as GitHub Actions CircleCI, and/or others.

Publication Options

• Do you wish to automatically submit to the Journal of Open Source Software? If so:

JOSS Checks

• The package has an obvious research application according to JOSS's definition in their submission requirements. 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: "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 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.

• Yes I am OK with reviewers submitting requested changes as issues to my repo. Reviewers will then link to the issues in their submitted review.

Confirm each of the following by checking the box.

• I have read the author guide.
• I expect to maintain this package for at least 2 years and can help find a replacement for the maintainer (team) if needed.

Please fill out our survey

• Last but not least please fill out our pre-review survey. This helps us track
  submission and improve our peer review process. We will also ask our reviewers
  and editors to fill this out.

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.

Footnotes

1. Please fill out a pre-submission inquiry before submitting a data visualization package. ↩

[lwasser]

hey there @robelgeda 👋 i suspect this is an astropy package. can you kindly confirm? if so i'll work on updating the template with the astropy "check" this week as well. cc: @isabelizimm (we can chat in slack about this too! it will be our first astropy package) 🚀

[robelgeda]

Yes it is and thank you!

[isabelizimm]

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.

• Installation The package can be installed from a community repository such as PyPI (preferred), and/or a community channel on conda (e.g. conda-forge, bioconda).
  • The package imports properly into a standard Python environment import package.
• Fit The package meets criteria for fit and overlap.
• Documentation The package has sufficient online documentation to allow us to evaluate package function and scope without installing the package. This includes:
  • User-facing documentation that overviews how to install and start using the package.
  • Short tutorials that help a user understand how to use the package and what it can do for them.
  • API documentation (documentation for your code's functions, classes, methods and attributes): this includes clearly written docstrings with variables defined using a standard docstring format.
• Core GitHub repository Files
  • README The package has a README.md file with clear explanation of what the package does, instructions on how to install it, and a link to development instructions.
  • Contributing File The package has a CONTRIBUTING.md file that details how to install and contribute to the package.
  • Code of Conduct The package has a CODE_OF_CONDUCT.md file.
  • License The package has an OSI approved license.
    NOTE: We prefer that you have development instructions in your documentation too.
• Issue Submission Documentation All of the information is filled out in the YAML header of the issue (located at the top of the issue template).
• Automated tests Package has a testing suite and is tested via a Continuous Integration service.
• Repository The repository link resolves correctly.
• Package overlap The package doesn't entirely overlap with the functionality of other packages that have already been submitted to pyOpenSci.
• Archive (JOSS only, may be post-review): The repository DOI resolves correctly.
• Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

---

• Initial onboarding survey was filled out
  We appreciate each maintainer of the package filling out this survey individually. 🙌
  Thank you authors in advance for setting aside five to ten minutes to do this. It truly helps our organization. 🙌

---

---

Editor comments

Welcome to the pyOpenSci community! We are so glad you are here! A few things to update on initial checks, but you all are in great shape. I really enjoyed running through your tutorials--not being someone in the astro space (haha!), I got such a great sense of accomplishment building models and making beautiful plots, even though I was just copy/pasting code 😆 Awesome work!

1. Update README.md to include installation guide
2. In the installation, there is a section that states:

Before installing PetroFit, you may need to install the required dependencies. You can do this using the requirements file located in the top directory of the repository. ...

package managers will install all the required dependencies of a package, as long as they are specified (in this package's case, in the setup.cfg in the install_requires section). If this is referring to installing tools for people to develop the PetroFit package, it should go in the CONTRIBUTING.md or Developer install instructions.

3. Add zenodo badge (I see one on the Citing and Credits) page in docs to README.md
4. Add a docstring to functions in the public API:
   • get_default_gen_sersic_bounds
   • petrosian_profile

Suggestions/random things I noticed (non-blocking to review):

• In ci_tests.yml, the prefix parameter looks to be unused. Also, it looks like you have matrix.allow_failure always set to false, so you could probably just set continue-on-error to be false. I'm not sure if this CI suite is something you plan to customize/expand more and these are set in anticipation of later changes, but at the moment it's quite repetitive.

I have never seen an Imposter syndrome disclaimer before, and I have to say it made me a little emotional--I love it so much 🤍 what an awesome note to add to a README!

[robelgeda]

Hi @isabelizimm, thank you for taking the time and your kind words! I am also sorry about the late reply, I wanted to see if I can update things quickly.

I found your suggestions very useful and have updated the main branch. For the installation, I would need to make a release to upload to pypi/pip but would prefer to wait until the review process is complete. Other than that, we should be all set. Here are some quick notes and comments:

• I added a quick section about installing using pip.
• Related to this, the package should be able to install using pip alone now. Astronomers tend to be a fan of requirement files especially because they sometimes have cluttered environments. That being said, I am a fan of not needing one for installation as well!
• I have added the zenodo tag to our README.
• I cleaned up the functions in models.py and added the docstrings as requested.
• Thank you for pointing out the ci_tests.yml items, I cleaned up the file! I kept the prefix for now with a place holder.
• I love that AstroPy has added the section about imposter syndrome, I believe it was first adopted by MetPy. I think its a challenge that often gets overlooked and one I am well acquainted with so I am proud to have it on our intro!

Thanks again!

[robelgeda]

Hi all, just checking in to see if we are are set with the pre-review stage? If not, we can make further changes!

[isabelizimm]

Yes! You are finished with this stage, and we have found an editor to lead the review! @dhomeier will be guiding you through the review process.

[robelgeda]

Hi @dhomeier, I hope all is well. I wanted to check in to see if we have found reviewers for the package. There is updates that we wanted to do but I wanted to get them started after the astropy review process. What does the timeline look like for this review so we can plan accordingly. - Thank you for your time.

[dhomeier]

@robelgeda I only had a response from one potential reviewer so far, but am still awaiting reply if/when they would be able to do a review. As we have just gone through a funding application for the project, and with our annual coordination meeting coming up next week, I don't expect any news before the second half of the month. I would not let yourself hold up any planned updates of your package; unless you are doing some major restructuring I am sure this would not interfere with the review (and even if, we could certainly find a solution, and having the reviews include your updates would be preferable anyway).
Apologies for the slow transition process and thank you for your patience!

[robelgeda]

Thank you for letting me know! and no problem! Most of the updates are new features so I will push them as they come. Also my timeline is on the scale of the remaining year so no worries. - Thanks again and looking forward to the feedback.

[robelgeda]

Hi @isabelizimm @cmarmo @dhomeier , I was wondering if we had any updates for this review request? Looking forward to your feedback!

[dhomeier]

Sorry for all the delays @robelgeda, and thank you for your patience!
I have now found two reviewers for your submission and expect the first comments in the next days, the second review within the following weeks.

[nden]

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README.
• Installation instructions: for the development version of the package and any non-standard dependencies in README.
• Vignette(s) demonstrating major functionality that runs successfully locally.
• Function Documentation: for all user-facing functions.
• Examples for all user-facing functions.
• Community guidelines including contribution guidelines in the README or CONTRIBUTING.
• Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.
  • author email is missing

Readme file requirements
The package meets the readme requirements below:

• Package has a README.md file in the root directory.

The README should include, from top to bottom:

• The package name
• Badges for:

  • Continuous integration and test coverage,

    The package has a CI badge but does not have one for test coverage. 
    

  • Docs building (if you have a documentation website),

  • A repostatus.org badge,
    There is no repostatus badge

  • Python versions supported,
    It does not have a list of supported Python versions.
    This is something that tripped me initially. I installed it in a Python 3.11 environment without any warnings or errors and it failed to import. After realizing that all tests use Python 3.12 and installing it in a 3.12 env it works.

  • Current package version (on PyPI / Conda).

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.)

• Short description of package goals.
• Package installation instructions
• Any additional setup required to use the package (authentication tokens, etc.)
• Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
  • Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
• Link to your documentation website.
• If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
• Citation information
  Citation information is included in the documentation. I recommend adding it to the README file to make it easily discoverable.

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:

• Package documentation is clear and easy to find and use.
• The need for the package is clear
• All functions have documentation and associated examples for use
• The package is easy to install

Functionality

• Installation: Installation succeeds as documented.

• Functionality: Any functional claims of the software been confirmed.

• Performance: Any performance claims of the software been confirmed.

• Automated tests:

  • All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
  • Tests cover essential functions of the package and a reasonable range of inputs and conditions.

• Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)

• Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
  Note: The package uses the now archived astropy template structure. It is very close to the pyOpenSci packaging guidelines. I recommend updating and removing the astropy specific bits.

  A few notable highlights to look at:

  • Package supports modern versions of Python and not End of life versions.
  • Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)
    The code formatting is standard and follows PEP 8 guidelines. However, running pylint generates a lot of messages pointing to issues which can be easily fixed (though they do not cause functional issues).

For packages also submitting to JOSS

• The package has an obvious research application according to JOSS's definition in their submission requirements.

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:

• A short summary describing the high-level functionality of the software
• Authors: A list of authors with their affiliations
• A statement of need clearly stating problems the software is designed to solve and its target audience.
• References: With DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

• The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 5

---

Review Comments

The package is in a very good shape and functional. Recommendations which will improve it are

• Add a list of supported Python versions to the README
• Add coverage test, report and badge
• Add a pylint Ci test
• Update the package structure to remove files from the old astropy template and modernize it according to the pyOpenSci packaging guidelines linked above

[dhomeier]

Thanks for your review @nden!
To chime in on the supported versions, the python_requires = >=3.12 is reflected on PyPI, and a git install with 3.10 or 3.12 gives me 0.5.0 instead; this however is not compatible with astropy 6.x.
I do not see any obvious changes in the code in 0.5.1 that would break compatibility with 3.10 or 3.11, so is it really necessary to stop supporting these versions already? Ultimately this is of course your decision, but it's foreseeable that this will lead to errors and confusion for users who simply try to install it in an existing 3.10 or 3.11 environment. Generally it's recommendable to stay in sync with the dependency and deprecation guidelines of SPEC 0 that Numpy (and Astropy) follow as well.

[dhomeier]

@robelgeda we have also checked the conditions for a JOSS publication following up on your paper in the AJ, and found that it is still possible to submit a publication focussing on the code 2 years later. So if you wish to still add a paper.md within the next 2 weeks, we can probably get it reviewed without much additional delay. If you do reference the AJ publication, you should probably point out changes since the version described there.

[kyleaoman]

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

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README.
• Installation instructions: for the development version of the package and any non-standard dependencies in README.
• Vignette(s) demonstrating major functionality that runs successfully locally.
• Function Documentation: for all user-facing functions.
• Examples for all user-facing functions.
• Community guidelines including contribution guidelines in the README or CONTRIBUTING.
• Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.

Readme file requirements
The package meets the readme requirements below:

• Package has a README.md file in the root directory.

The README should include, from top to bottom:

• The package name
• Badges for:
  • Continuous integration and test coverage,
  • Docs building (if you have a documentation website),
  • A repostatus.org badge,
  • Python versions supported,
  • Current package version (on PyPI / Conda).

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.)

• Short description of package goals.
• Package installation instructions
• Any additional setup required to use the package (authentication tokens, etc.)
• Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
  • Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
• Link to your documentation website.
• If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
• Citation information

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:

• Package documentation is clear and easy to find and use.
• The need for the package is clear
• All functions have documentation and associated examples for use
• The package is easy to install

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests:
  • All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
  • Tests cover essential functions of the package and a reasonable range of inputs and conditions.
• Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
• Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
  A few notable highlights to look at:
  • Package supports modern versions of Python and not End of life versions.
  • Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)

For packages also submitting to JOSS

• The package has an obvious research application according to JOSS's definition in their submission requirements.

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:

• A short summary describing the high-level functionality of the software
• Authors: A list of authors with their affiliations
• A statement of need clearly stating problems the software is designed to solve and its target audience.
• References: With DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

• The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:

4

Review Comments

I've now finished my review. Overall the package seems to be in reasonably good shape. There are a few things that I think should be addressed before I sign off:

• readme badges PetroFit/petrofit#213
• Code Linting in Continuous Integration PetroFit/petrofit#214
• citation information should be included in readme PetroFit/petrofit#215
• include example(s) in readme PetroFit/petrofit#216
• local docs build via tox fails PetroFit/petrofit#218
• [BUG] Automated tests fail locally PetroFit/petrofit#236

I'll also highlight an existing issue from the authors PetroFit/petrofit#104 : there are tests and they do cover the essential functions of the package at a level adequate to pass review, but the overall/long-term health of the package would probably benefit from better test coverage.

I've also kept some notes of other things that I've noticed, but for the purpose of this review these should be considered optional:

• Add PR & issue templates PetroFit/petrofit#212
• Miscellaneous issues in Docs PetroFit/petrofit#219
• Misc fixes noticed during PyOpenSci review PetroFit/petrofit#220

[robelgeda]

Thank you @kyleaoman , @nden , and @dhomeier !!! I will try to address a bulk of these this weekend. These are very useful points so thanks again for your time and effort!

[dhomeier]

Looking forward to your updates @robelgeda!
Note on the citation information: Astropy has already recommended using a CITATION file for some time; we are currently discussing how to best update the pyOpenSci packaging guide to reflect this. A useful addition would certainly be a link in the README to the former file.

[robelgeda]

@dhomeier the review was very useful for the project! I have added all the changes recommended, except some minor docstring updates, in the main branch. The biggest change is moving to using pyproject file with hatch for package management. I plan to spend more time on docs (and docstrings) once we have refactored the code to contribute upstream. That being said I have run black to comply with some of the recommendations suggested in this review process.

For the JOSS submission, I think I will come back to it after this review since there are plans to introduce JAX based fitting code in the very near future (at least a branch working toward this goal). For the purposes of this review, I think its okay for us to process this application without the journal submission.

Ref Milestone: https://github.com/PetroFit/petrofit/milestone/6?closed=1

Please be sure to look at the latest RTD branch, for the duration of the review, I have set the default to sable for users who are visiting the docs.

If this looks okay, I am ready to release this version of the code.

CC @nden and @kyleaoman

[robelgeda]

I have also added a Generative AI Policy section to our README. This might not be common practice at the moment but it is important to supervise code added to our repo given that its main goal of our code is scientific precision and ideal levels of accuracy. Generative AI can be used for docs, GitHub workflow (tickets, PRs...) and reports freely since these things are used to communicate and document changes to other users/admin as opposed to affecting the core function of the codebase.

[dhomeier]

Thanks @robelgeda for the extensive updates! @kyleaoman, @nden, it looks like all issues with the submission, especially those itemised under Review comments and in Petrofit's issues #213-220 have been resolved with the exception of some linting aspects, in particular involving docstrings.
Could you re-review the package to confirm that all mandatory criteria are now met? Thanks!

[kyleaoman]

Hi @dhomeier , @robelgeda . I had a look at the changes. Mostly looks good, but the updated test suite still fails, I've opened a new issue (added a link in my report).

Also not quite sure what to do with the docs item - if it's part of the pass/fail requirements for review then it should be done before passing, not promised for later. Some of my comments/suggestions for the docs are outside of the pass/fail scope, and those it's fine to defer of course, just trying to work out where to draw the line. I flagged a few functions with missing docstrings, and the tickbox on the review checklist says "function documentation for all functions user-facing functions". I suggest those get filled in to pass review from my side, if @dhomeier agrees.

Other than that everything looks pretty good to me!

[dhomeier]

Thanks for the quick reply @kyleaoman! I had missed that you had also flagged functions entirely without docstrings, and agree this is more problematic than the docs not yet conforming to a consistent numpydoc or other style.
The pyOS Packaging Guide asks for all of the user-facing API to be documented within reason, so there is certainly room for tolerance, but any function important for the normal workflow of using the package should have a docstring.
The case of the failing test with PSFConvolvedModel2D.evaluate
https://github.com/PetroFit/petrofit/blame/af30a253fb1ec45fd236e46d46230f81f0f21c8e/petrofit/modeling/models.py#L353-L356
seemingly not called with valid psf_p type may in fact emphasise this, the missing docs making it harder to see how *params should be set correctly.