Astropy Affiliated Package Review

[astrofrog]

This package has been re-reviewed by the Astropy coordination committee in relation to the Astropy affiliated package ecosystem.

We have adopted a review process for affiliated package that includes assigning quantitative ‘scores’ (red/orange/green) for different categories of review. You can read up more about this process here. (This document, currently in Google Docs, will be moved to the documentation in the near future.) For each of the 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
At the time this was reviewed, it looked like integration with Quantity/Units could be improved. In addition, at the moment halotools doesn't seem to use astropy.modeling, but there may be good reasons why this is not the case. For now we have therefore scored this as 'Partial', but feel free to clarify if you think that the way it is currently could not be improved.
Documentation
No further comments
Testing
No further comments
Development status
No further comments
Python 3 compatibility
No further comments

Summary/Decision: Things are looking good! There are a few areas where things could be improved as described above. However, these aren't critical and this package still meets the criteria to be an affiliated package. Keep up the good work!

If you agree with the above review, please feel free to close this issue. 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. Note that we are in the process of redesigning the http://affiliated.astropy.org page to show these scores (but not the comments). Finally, please keep the title of this issue as-is (“Astropy Affiliated Package Review”) to make it easy to search for affiliated package reviews in future.

[aphearin]

Many thanks for re-reviewing the code and providing feedback, @astrofrog and co.

Re: Integration with Astropy ecosystem

1. Astropy Quantities remains on the wish-list of things to improve before the next major release of v1.0 (the main new ingredient of v1.0 is to make the package scalable to very large number of cores, which will take considerable time). I agree the package would be appreciably improved by using Units, which are quite nice. Currently, all the units are hard-coded to be the same throughout the package, with lots of repetitious and explicit statements of the units in all the docstrings. That is admittedly quite clunky. However, if I were to use to Units, I would still need all that hard-coding and repetitious documentation to make the little-h convention explicit in how units like stellar mass are defined, since this feature is currently not supported by Quantities (or has this changed?). So that does limit some of the benefit of migrating to Units. This is not a deal-breaker, I will keep Units on the v1.0 feature list, I'm just providing feedback/explanation. If no one is working on that feature in the astropy development team, I should probably sign up for the task myself.
2. I have not followed recent developments in astropy.modeling, though @eteq and I discussed this while the empirical_models sub-package was being developed, and at that time the flexibility we needed was not supported by the modeling features. This is certainly worth revisiting, so I raised Revisit whether empirical_models can be wrapped by astropy.modeling #824 as a reminder to look into this.

[astrofrog]

@aphearin - thanks for the explanations! I think for now it probably sounds like we can leave integration as 'partial' - and if units/modeling don't provide enough flexibility, please do feel free to open issues on the core astropy repo with specific requests!