search

Chandra-MARX / marxs

Multi-Architecture-Raytrace-Xraymission-Simulator
http://marxs.readthedocs.io/en/latest/
GNU General Public License v3.0
6 stars 9 forks source link

Astropy Affiliated Package Review #167

Closed astrofrog closed 6 years ago

astrofrog commented 7 years ago

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/ScopeSpecialized%20package
No further comments
Integration with Astropy ecosystemGood
No further comments
DocumentationGood
No further comments
TestingGood
No further comments
Development statusHeavy%20development
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.

hamogu commented 7 years ago

Comment: While not complete, there is considerable testing for correctness in the testing suite, e.g. in https://github.com/Chandra-MARX/marxs/blob/master/marxs/optics/tests/test_grating.py where most of the tests compare results to the known good analytical solution of the grating equation or in https://marxs.readthedocs.io/en/latest/examples.html#polarization where simulation results are compared to experimental data.

I also want to remark that test coverage is higher when tests are run locally, because visualization.mayavi cannot even be imported on headless serves like Travis. (It's technically possible to set that up, but it's not easy.)

astrofrog commented 6 years ago

@hamogu - thanks for the clarification! We'll take a look at the testing aspect again. Regarding visualization.mayavi, have you tried setting SETUP_XVFB=True on Travis? (should allow GUIs/VTK/OpenGL to run)

astrofrog commented 6 years ago

@hamogu - after discussing this with the coordination committee we've decided to bump up your 'test' score to green :) However I'd still encourage you to look into using SETUP_XVFB=True on Travis to try and run the mayavi-related tests.

hamogu commented 6 years ago

@astrofrog I did not know about the SETUP_XVFB option. I looked into installing xvfb myself, but decided that it's not worth it to require sudo=True just for this case.

That shows how much packages can profit from an independent review every now and then, particularly small packages that don't have the same range of experiences in the developer community that astropy itself has. (In this case, it's me with small contributions from two undergraduate students in their summer projects).

bsipocz commented 6 years ago

@astrofrog I did not know about the SETUP_XVFB option. I looked into installing xvfb myself, but decided that it's not worth it to require sudo=True just for this case.

@hamogu - what do you mean by requiring sudo=True, you don't need sudo for using SETUP_XVFB=True

hamogu commented 6 years ago

@bsipocz : Right, this is exactly what I said "I did not know about the SETUP_XVFB option." Instead (and that is a word I should have added to my previous comment, but did not) I looked into installing xvfb myself, but decided that it's not worth it...".

I was under the impression (but that might be wrong, too), that I need to do sudo = True, if I want to sudo apt-get install xvfb and set DISPLAY .... So, thank you to @astrofrog who pointed out that I can just try SETUP_XVFB=True. Now, fortunately, that mean it does not matter any longer if I am right or wrong about sudo for apt-get install, because there is this other pathway to hopefully get what I want.

bsipocz commented 6 years ago

Ah, ok, sorry for misunderstanding :)