Adding pypeit

[profxj]

We are requesting affiliated status for our data reduction pipeline
package: PypeIt.

Please consider it.

[hamogu]

Thank you for proposing this package as an affiliated package! I'm happy to confirm that your package is now under review and we'll post the results of the review here and on the mailing list.

[hamogu]

This package has been reviewed for inclusion in the Astropy affiliated package ecosystem by a member of the Astropy 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
Reviewing affiliated packages. For each of the review categories below we have listed the score and have included some comments when the score is not green.

Functionality/Scope
No further comments
Integration with Astropy ecosystem
PypeIt is presented basically a black box, which has a number of screws and switches to adjust its work. The API documentation is not very useful. Also, there is no interface to the Spectrum1D class from specutils. Spectra are quite basic for astronomers and a need to use different Python classes for them depending on the package is a pain. Together, that makes is almost impossible to use PypeIt as part of the wider astropy ecosystem. Code re-usability to avoid duplicate work is an important goal for the Astropy project. We understand that this is, to a certain degree, a design choice to make PypeIt run as a pipeline instead of as a framework that other programs can build on top. However, even small documentation improvements could be a big help for users trying to use just part of Pipit in a Python program. For example, module level docstrings for the submodules listed at https://pypeit.readthedocs.io/en/release/api/modules.html can summarize in a in few lines of text what a module is for (some submodules don't have docstring at all, in others it's so short that it's hardly useful) and the submodules listed could be grouped in some logical, instead of just alphabetical order.
Documentation
The documentation looks great and well-organized. A nice point is that they start with the CoC in "Getting started"
Testing
The package uses pytest, but has < 50 % code coverage, presumably because the rest of the code is tested using the pytest-development-suite which just runs too long for CI. Developing more units tests that run and test basic functionality on small, artificial test data for a larger part of the code could help to "fail early" and catch problems in new PRs before running the full development suite.
Development status
Package is healthy, has stable versions, is maintained by a community with several long-term contributors.
Python 3 compatibility
No further comments

Summary/Decision: Thanks for your work on this package! At the moment, we
found some issues with the astropy ecosystem integration. As per the review guidelines, we
therefore won't be able to accept this package as an affiliated package yet.
We will leave this pull request open for six months in case you would like to
respond to the comments and/or address any of them; please let us know by a comment on this PR.

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 affiliated package editors
listed on the Astropy team page,
and we will do a new review.

Edit: Corrected two misspellings of the package name.

[profxj]

Thanks for the review. And thanks for passing us on the majority of items.

Regarding the one “Red” item, here are some things for you to consider further:

• PypeIt was written sufficiently general to enable anyone in the community to add new spectrographs (or setups). This has happened several times already. Therefore, as regards the stated philosophy of “Code re-usability to avoid dublicate work is an important goal for the Astropy project”, you might be hard pressed to find a better example than PypeIt in production. Here is an example PR of one of those additions:

  • https://github.com/pypeit/PypeIt/pull/1216/files
  • Note how few new lines and files were required for this new telescope+spectrograph, i.e. many 10,000s lines of codes were reused for this new application.
  • Also, we provide somewhat extensive documentation to enable the community to add any new spectrograph (or sub-component): https://pypeit.readthedocs.io/en/release/new_spectrograph.html

• The first point aside, we have made a concerted effort from the start to separate out codes providing package-independent functions/methods for general use. There are 35000 lines of code in core/. These modules were intentionally written to take simple Python objects and return simple Python objects. There are a few exceptions (e.g. the input is occasionally one of our PypeItPar classes), but they are rare.

  • Furthermore, we have made a sustained (and continuing) effort to provide a complete doc string for each. A quick scan gives an estimate of ~80% completeness. As you likely appreciate, providing such documentation is a huge endeavor.
  • These codes are primarily for spectroscopic reduction but many can be used for standard imaging as well.

• There are 28000 lines of code in the folder above which contains Classes that you might be considered specific to PypeIt.

  • However, several of the files (e.g. utils.py) could/should be moved to core/ and these contain over 20% of the lines.

We ask that you reconsider the “Red” rating or provide more concrete requests for us to achieve the next level. It is difficult for us to scope out the work that would be required from the review.

Last, please refrain from referring to the code base as “PipeIt” or “Pipit”. Thanks!

Adding @eteq

[hamogu]

We appreciate your thorough and detailed feedback. I apologize for messing up the program name; in the future I will copy and paste the string from one of your official documentations to make sure I get the right combination (a problem I have for many code names, including my own).

As for a concrete request, let me reword and expand the comment under the item listed red:

• Module level docstrings for the submodules listed at https://pypeit.readthedocs.io/en/release/api/modules.html can summarize in a in few lines of text what a module is for. Some submodules don't have docstrings at all, e.g. https://pypeit.readthedocs.io/en/release/api/pypeit.fluxcalibrate.html, in others it's so short that it's hardly useful, e.g. what does "create models of arc lines" in wavemodel mean? Do you create 2D models of where the arclines fall spatially on an image? Do you have a physical model what arclines will be emitted by the lamp based on temperature and type of the arc lamb? Of course, docstrings are never fully complete and there is always a way to misunderstand them. In Astropy, we don't have a set minimum number of words that a docstring must have, but then we also have extensive narrative docs that explain how to use almost every class and function, PypeIt does not have that because it operates different and that's OK. However, that means that the docstrings in the API sections of the docs are the only entry point for developers that want to use PypeIt as a framework instead of just running through a specific pipeline from the command line. I'm fully aware that documenting everything to that level is a huge amount of work and may not be the best use of developer time for a package like PypeIt.
• submodules listed could be grouped in some logical (e.g. the order they would be used in a typical pipeline workflow), instead of just alphabetical order or there could be some explanatory text/flow-chart what submodules do. Without knowing the code, I cannot write that text. It need not be long, just some general ordering or overview. For example, the submodules history, pypeit, pypeitsetup, pypmsgs seems to be designed to run the pipeline, but might be less interesting for people who look for the concrete implementation of some particular algorithm. On the other hand coadd2d and sensfunc would be. Alternatively (but that's of course a lot more work and requires a lot more discussion and coordination between developers, submodules could be nested more like pypeit.wave.calib, pypeit.wave.model or pypeit.pypeit, pypyit.pypyit.setup, pypeit.pypeit.conf ...). Again, that does not matter for users that use PypeIt as a fully autonomous pipeline from the command line and control it with a configuration file, but would be a big help for users who just need a single algorithm/step (e.g. trace a spectrum in 2D). (It would also help users who need to debug some particular problem for their dataset or non-standard instrument or who want to add some new algorithm for just one step in the pipeline.)

For all those points, I, as an external reviewer, can't be the person to decide how exactly this can be best expressed in the existing codebase. If I knew, I would open a PR! However, I believe that a relative small effort of just a few hours can make a big difference here (e.g. sort the sub-module list and expand docstring 3-5 lines of text and/or one short example using the mode important class/function for each sub-module module).

• Relation to specutils pypeit/PypeIt#1263 is open, but it's not clear what do do about it (I don't know either). I'm not requesting to solve that right now to become affiliated; I just want to point out that integration and re-usability is not only about the structure of the docs. I've merely concentrated on that above because I think that that can be improved with relatively little investment of time and would be sufficient to make PypeIt an affiliate package.

Lastly, I really want to congratulate you on this package. PypeIt is truly a humongous effort that provides great benefit to a wide variety of users.

[profxj]

Ok, this is very helpful and tractable.

We will endeavor to achieve what you suggest and
will get back to you when we have done so.

Thx