Proposed affiliated package: PyNeb

[Morisset]

PyNeb is a modern python tool to compute emission line emissivities (recombination and collisionally excited lines) for nebulae. Its first version is from 2012 and is in constant development since then. It is widely used (more than 120 citations for the papers describing it). PyNeb current version is 1.1.9.

[astrofrog]

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.

[Morisset]

Hi,
I do not understand what I'm supposed to do with the red "All check have failed" message below. It seems that the failure is because I did not filled something that the reviewer is supposed to do, not me. It also seems that all the other package waiting to be in astropy have the same problem. Then perhaps the problem is not on the package side, but rather on the design of this interface? Can somebody tell me what to do?
Thanks a lot,
Christophe Morisset

[bsipocz]

@Morisset - no worries about the CI for now, changes has happened in the main repo since. Fixing these can wait until the review is done.

[Morisset]

Thanks @bsipocz Do you have any idea of the time the review will take? Thanks. Christophe

[astrofrog]

With apologies for the delay, 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
PyNeb does not integrate much with astropy - it only uses astropy.io.fits and astropy.table to read input tabular data. There are many places where the package could integrate better - for example, functions could take temperatures and densities as quantities and results could have units and return quantities. While internal calculations may be done in pure numpy arrays for speed, at least input / output would profit from units (a review of the mailing list shows that users frequently put in wavelength in the wrong units or are not clear if errors are relative or absolute). Another example is the "Observation" class. This could be an astropy Table class with a little metadata plus some convenience functions, or it could be an object inheriting from Table, which would save hundreds of lines in the PyNeb implementation and offer a lot more input/output formats for a table with no extra work (e.g. LaTeX for publications).
Documentation
At the moment, the user manual is almost exclusively in notebook form, and there is no connection between the manual and the API docs in the reference manual. Ideally, when users read the narrative documentation, it would be good to make sure the method/function/class names in the narrative can link to the API docs so that users can find out more about the available options. For consistency with other packages in the astropy ecosystem, you might want to consider using Sphinx as the primary framework for the docs, which would allow you to have API docs (using e.g. sphinx-automodapi) and which also allows notebooks to be used as-is to generate the docs (using nbsphinx). In this way, all the docs would be on a common site rather than be separated into the reference API docs and the user manual. Aside from the framework, the notebooks could do with more explanatory text. It doesn't look like the documentation is automatically built/tested when changes are made to PyNeb. On the plus side, the mailing list is active and user support there is fast.
Testing
The test coverage, as measured by pytest pyneb --cov pyneb, is around 21%, which is very low - it would need to be significantly higher for the package to be accepted. In addition, we would highly recommend setting up a CI service (Travis, AppVeyor, CircleCI, Azure Pipelines, etc.) to run the tests any time a change is made to the repository.
Development status
It looks like development activity is low at the moment
Python 3 compatibility
No further comments

In addition to the comments above, the API for PyNeb is a little inconsistent with common practices for Python packages, in that function/method names use Java-like syntax (e.g. getOmega instead of get_omega). While not critical for the acceptance of the package, we would highly recommend considering making the API of the package compliant with the PEP8 style guidelines.

Summary/Decision: Thanks for your work on this package! At the moment, we found some issues in some of the review areas. 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 a month in case you would like to respond to the comments and/or address any of them.

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.

[Morisset]

HI,
Thanks a lot for having taking time to review PyNeb for integration to astropy. We started to develop this code in 2011, when astropy was embryonic. The object structures have been defined at that time, and can barely be changed to fit some of the criteria that you seem to consider important (Observation object is reading observations, it is not aimed to be used to publish them, we do not need to have a Table inheritance). The output units have been defined in the docstings, and we are not sure that adding units using astropy will bring more benefits than issues. Anyway, the too small manpower associated to PyNeb is not able to add a documentation in the sphinx format as you ask for.

[astrofrog]

@Morisset - thanks for your reply to the review and apologies for not replying to you sooner. For now I will close this pull request, but if you do find time to address the documentation and testing issues we highlighted, you would be very welcome to apply again - at the end of the day the only real blocker to acceptance as described in the review was the testing coverage (packages with 'orange' status on some of the items can still be accepted).