Ensure that existing manipulation and analysis functions use the `mask` where appropriate.

[eteq]

This comes out of #509, which introduces a function that computes and assigns mask's to Spectrum1D objects. This led to a discussion where it was realized that a lot of the downstream analysis tasks that would follow on from #509 need to respect the mask for #509 to actually be practically useful - without that it seemed like a flux_masked or similar seemed necessary, which is not desirable since it essentially replicates the mask in Spectrum1D itself.

So the point of this issue is to make sure the currently-existing analysis and manipulation tools are mask-aware. At the time of writing this, I think the current status is:

• specutils.analysis functions: centroid, equivalent_width, fwhm, gaussian_fwhm, gaussian_sigma_width, line_flux, snr - all of these ignore the mask entirely, but I think it's a pretty easy thing to make them work: if the mask is None, do what it does now, otherwise, just do self.flux[~self.mask] where self.flux currently appears (and self.spectral_axis -> self.spectral_axis[~self.mask], although they don't all use the spectral axis).
• line and continuum fitting (same underlying implementation. in fit_lines): I think this is also basically a one-liner in the implementation, doing essentially the above, but in the call to the fitter.
• parameter estimators in @brechmos-stsci may be able to advise here, but I think these are all eventually implemented via the analysis functions, right? So then if we fix those then it automatically carries over to the estimators?
• extract_region: already does the correct thing of just carrying through the mask.
• arithmetic: this already does the right thing by acarrying the mask thorough, I think by virtue of leaning on the existing NDData machinery.

The one question in my mind that applies to all of the above: do we want use_mask to be a keyword we add to all of these? My initial inclination is no: we always use the mask, but if the user does not want that, they can just set the mask to None. Or alternatively, we add a without_mask method to Spectrum1D that just copies the spectrum but sets the mask to None. Then the user could just do things like function(spectrum.without_mask() instead of function(spectrum, use_mask=False. Any preferences from others? If not I think I like the without_mask option.

cc @brechmos-stsci @camipacifici @nmearl

[eteq]

Oh and since I already looked at it, I'll make a PR for fit_lines.

[hcferguson]

This looks sensible to me.

Regarding without_mask, it seems like there should be some way of doing this without copying any data. In the case where the user substitutes the mask with None, there is no copy. But it's inconvenient for the user to get the mask back. It would be nice to be able to toggle the mask on and off, which is what you are getting at, but do it by just temporarily changing the reference of the mask attribute.

If it were possible, it might be simpler for there to be spectrum.unmasked as a version of the same spectrum1D instance, but with the mask set to None. I couldn't instantly think of a way to implement this though. So calls would be function(spectrum.unmasked) rather than function(spectrum.without_mask()).

[eteq]

If it were possible, it might be simpler for there to be spectrum.unmasked as a version of the same spectrum1D instance,

Hmm, one option would be to do a shallow copy - e.g., it would be a copy of the Spectrum1D object, but the flux (and spectral_axis, uncertainty, etc) objects would still be referring to the same arrays. Then it's still a copy but a fairly "lightweight" copy. Do you think that would work, @hcferguson ?

(in fact I guess without_mask could have a keyword to choose between shallow and deep copies... Although probably the default should be shallow for those who aren't thinking too hard about it?)

[hcferguson]

Might work. But I think that X = spectrum.whatever() makes you think you are getting a deep copy. Whereas X = spectrum.whatever makes it clearer that you are just getting a sort of a view. But I can't think of another example where an attribute basically refers to a different version of the instance itself -- with one of its other attributes modified -- so this may be equally confusing.

[jdavies-st]

Does #670 resolve this issue @ibusko? If so, this issue can be closed.

[rosteen]

@jdavies-st @ibusko I'm still seeing line_flux and equivalent_width returning NaN [unit] on masked arrays after #670, can someone else test and see if they're getting the same result? I assume they were working to merge that PR, but I haven't had time to track down exactly why it's failing. centroid, gaussian_fwhm and gaussian_sigma all work in my testing (I'm using these in a line analysis plugin in specviz).

[nmearl]

@rosteen can you provide an example of what you're doing? I'm playing around with the analysis functions, but am not getting any NaN values returned.

[rosteen]

I'm using the specviz UI to define a subset around a line, then using data = specviz.app.get_data_from_viewer("spectrum-viewer", 'Subset 1') to retrieve the data via the glue-astronomy data translators (which I've updated locally to return the mask values that specutils expects). Then I'm just calling specutils.analysis.line_flux(data["Subset 1"])

I attached zipped numpy save files for the flux, spectral_axis and mask for replication. Load them into numpy as flux, spec, and mask, and then run the following to replicate:

flux = flux*u.Unit('1e-17 erg / (Angstrom cm2 s)')
spec = spec*u.AA
spec1d = Spectrum1D(flux=flux, spectral_axis=spec, mask=mask)
analysis.line_flux(spec1d)

mask_test.zip