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

Astropy Affiliated Package Review #555

Closed
astrofrog opened this issue Sep 26, 2017 · 3 comments
Closed

Astropy Affiliated Package Review #555

astrofrog opened this issue Sep 26, 2017 · 3 comments

Comments

@astrofrog
Copy link
Contributor

This package has been reviewed by the Astropy coordination committee for inclusion in 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/ScopeGeneral%20package
No further comments
Integration with Astropy ecosystemPartial
There are places where ginga could be better integrated with astropy, for example it could use the astropy implementation of zscale, as well as the scale and interval classes.
DocumentationGood
No further comments
TestingPartial
When trying to run tests locally, coverage came out very low, but it wasn't clear if this is real. It might be worth setting up automated coverage results on coveralls as other packages in the astropy ecosystem have done.
Development statusGood
No further comments
Python 3 compatibilityGood
No further comments

Summary/Decision: Things are looking good, and this package meets the review criteria for affiliated packages, so we are happy to confirm that we'll be listing your package as an affiliated package! Keep up the good work, and we encourage you to improve on the areas above that weren't “green” yet.

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.

@pllim
Copy link
Collaborator

pllim commented Oct 6, 2017

This is fine by me. @ejeschke , if you are okay with this review, you may close the issue. Thanks.

@ejeschke
Copy link
Owner

ejeschke commented Oct 7, 2017

I've been swamped, but I'll take a quick review of this next week and we'll close it then.

@ejeschke
Copy link
Owner

Closing. I agree with the test coverage part. We need more tests, and with that the code coverage will be increased. There are multiple back ends, and there are the usual issues with unit testing a GUI, but perhaps some of the more important tests might be handled by rendering to png images, since much of the rendering is toolkit agnostic.

Regarding the astropy integration, I think it is already quite good. It uses astropy pretty much as the default package for astronomy-related things and uses the astropy-helpers package. Ginga is a toolkit for making scientific viewers in general (not just astronomy--see paper and talk at SciPy 2013), so the fact that astropy is not used absolutely everywhere it possibly could is not surprising or necessarily a detriment.

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

No branches or pull requests

3 participants