Repository navigation
statmorph package re-review #421
Description
Hello,
I recently made several improvements to the statmorph affiliated package and I believe that the scores can now be updated. The original PR and review can be found here.
I have implemented the vast majority of the comments and recommendations from the review, which was very helpful. Please find below my response to the points raised in the review.
Thanks,
Vicente
Integration with Astropy ecosystem
- I have removed the custom normalize() function and replaced it with astropy.visualization.simple_norm().
- As suggested, I have replaced scipy.ndimage.convolve with astropy.convolution.convolve. However, regarding the FFT versions, there would be a significant performance cost in replacing scipy.signal.fftconvolve with astropy.convolution.convolve_fft. The latter seems to be about an order of magnitude slower (more details here and here). This is perhaps the most time-consuming part of the code (during the Sersic profile fitting), so I would strongly favor keeping scipy.signal.fftconvolve for now.
- I have replaced the custom _mode() function with photutils.background.ModeEstimatorBackground.
- Regarding the custom _quantile() function, I did not use numpy.percentile() because it sorts the input array, while _quantile() assumes that the array is already sorted. Therefore, _quantile() is infinitely faster than numpy.percentile() for sorted arrays (because it skips the sorting), which is always the case when _quantile() is called in statmorph. This represents the other most time-consuming part of the code (the calculation of the "multimode" statistic), so it is important to keep this custom _quantile() function.
Documentation
- As suggested, I have embedded the notebook tutorial into the documentation using nbsphinx.
- The public function utils.normalize() was removed, while utils.get_ax() was made private.
Testing
- The package now has a more extensive test suite that achieves 96% coverage.
- As suggested, the package now has CI running (GitHub Actions).
Python 3 compatibility
- I have dropped support for Python 2.
Activity
pllim commentedon Jan 14, 2021
This package has been (re)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.
astropy.convolution.convolve is now used. astropy.convolution.convolve_fft was not used because it is reportedly 10x slower, but turning off certain options would result in a huge performance improvement. Either astropy.convolution.convolve_fft or scipy.signal.fftconvolve is acceptable, especially if the extra features astropy provides are not needed.
photutils.background.ModeEstimatorBackground is now used.
The author's custom _quantile() function is faster because it does not need to sort the data. The input data are already sorted (this was missed in the last review), so this makes complete sense.
utils.normalize() was removed and get_ax() is now a private function.
The package is now using GitHub Actions for CI. However, the CI is triggered only for push and scheduled events, not pull requests (see .github/workflows/ci.yml#L3). It would be better if CI was enabled for pull requests to run the test suite on user contributions before the code is merged.
The
utils/image_diagnostics.pymodule has a hard dependency onmatplotlib, butmatplotlibis not listed as a required dependency, nor is it listed in the setupinstall_requires. Alternatively,matplotlibcould be made an optional dependency by moving thematplotlibimports inside themake_figurefunction.Summary/Decision: Everything looks great, and we're happy to confirm that this package maintains its status as an affiliated package! 🏆
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.
p.s. Previous review available at #315 (comment) .
vrodgom commentedon Jan 17, 2021
Thanks! I have implemented the last few comments (about CI and package dependencies).
pllim commentedon Jan 17, 2021
Awesome. Thanks, @vrodgom !