Sub-pixel precision for calculate_fwhm #484
Conversation
|
The tests are kinda flaky sometimes. I restarted it twice and the second time it passed :) |
| argmax = flux_value.argmax() | ||
| halfval = flux_value[argmax] / 2 | ||
| left = flux_value[:argmax] < halfval | ||
| right = flux_value[argmax + 1:] < halfval |
There was a problem hiding this comment.
Slices on numpy arrays are none inclusive on the upper range. Are you purposely not including the highest flux value, or did you mean something like this instead:
| right = flux_value[argmax + 1:] < halfval | |
| right = flux_value[argmax:] < halfval |
There was a problem hiding this comment.
Hi, yes - I was excluding it because flux_value[argmax] == halfval, so there's no point in including it during the comparison operation.
There was a problem hiding this comment.
sorry, I meant flux_value[argmax] = halfval * 2
|
This looks good, but you'll need to rebase for the latest changes in master. |
|
I'm having some difficulty again with the CI connecting to zenodo.org with 404 not found errors. Is there a way to rerun the tests without closing and reopening the PR? |
| argmax = np.argmax(flux) | ||
| halfval = flux[argmax] / 2 | ||
| # The .value attribute is used here as the following algorithm does not | ||
| # use any array operations and would otherwise introduce a relatively |
There was a problem hiding this comment.
Do you really mean "any array operations" here, or rather "Quantity" or "as a property", because flux.value is a np.ndarray and you are using indexing operations etc.?
specutils/tests/test_analysis.py
Outdated
| # since the maximum value occurs at wavelength=1, and the half-value of | ||
| # flux (0.5) occurs at exactly wavelength=2, the result should be | ||
| # exactly 1 (2 - 1). | ||
| assert result == 1.0*u.um |
There was a problem hiding this comment.
How about modifying that test (or adding another one) so that wavelength=2 is not actually on the grid, to test the interpolation? E.g.
wavelengths = np.linspace(1, 10, 31) * u.um
There was a problem hiding this comment.
@dhomeier can you open another issue to address expanding on the unit testing? I'd like to merge this PR now for a release and iterate on it over time.
There was a problem hiding this comment.
Sure. Just wanted to note that the PR adds some functionality for which there is no test coverage now, but if it has time I can look into it sometime.
There was a problem hiding this comment.
@dhomeier, I've gone ahead and included a test for this interpolation case. No need to make a new issue.
Improvement to the existing algorithm using linear interpolation to achieve sub-pixel precision.
Fixes #378