Make line/continuum fitting work in the mask case (and other mask-related tests) #519
Conversation
specutils/fitting/fitmodels.py
Outdated
| # TODO: really the spectral region machinery should have the power | ||
| # to create a mask, and we'd just use that... | ||
| index_spectrum = Spectrum1D(spectral_axis=spectrum.spectral_axis, | ||
| flux=np.arange(spectrum.flux.size).reshape(spectrum.flux.shape)*u.Jy) |
There was a problem hiding this comment.
is the Jy unit addition needed? That adds a small performance hit and is weird for what should be an integer array to begin with (on which note, should this be arange(..., dtype='int')?)
There was a problem hiding this comment.
The reasoning for this is that Spectrum1D expects flux inputs to be quantities - i.e., you can't give something that's unitless. The choice of Jy is arbitrary, as really any quantity will do, but it seemed safest to use an actual flux. But you're right it makes sense to keep it as an int so I can switch to the explicit u.Quantity(arr, u.Jy, dtype=int)
As the "HACK WARNING" indicates this is defintely a hack, though I think it's probably better just to try to do the "TODO" ASAP instead beyond optimizing this any more.
Co-Authored-By: Adam Ginsburg <keflavich@gmail.com>
|
I think I've addressed everything, so this should be good to go once the tests pass if @keflavich or @nmearl are willing to approve/merge it. |
|
Looks like there's a hold over from the recent |
|
@nmearl - Re the mexicanhat failure: I don't think it's related to this PR. I was about to added it but I realized we should treat it separately because I think we may want to leave the "old" name in for the Astropy 3.x compatible version. So I made a separate PR #564 to address that and propose this be merged separately. |
|
Just to clarify: this PR would be in for the astropy 3.x release, but the PR #564 would be included in the astropy 4.x release? |
|
@nmearl - yep, exactly! |
|
(since the test failure is unrelated I think this is good to go, as I'm guessing from the comments that @nmearl has looked at it already) |
|
This PR actually introduced a bug that is being fixed at #1164 . FYI |
This is the PR I promised in #516. It fixes the line-fitting machinery to ignore any masked parts of the spectrum when doing a fit.
Note this also includes a couple of tests I added in process of the above to ensure the masks behave correctly for arithmetic and spectral regions.
cc @brechmos-stsci @camipacifici @nmearl
(This may particularly benefit from a review by @brechmos-stsci who did a fair amount of the fitting stuff)