astropy / astropy/specutils

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

Open
#516 14 comments 0 reactions 0 assignees View on GitHub
analysis enhancement manipulation
Dominant language
Python
Stars
204
Forks
134
Avg merge
12h 55m
Merged PRs (30d)
1

Description

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

Contributor guide

Open the contributing guide

Research direction

Start with the listed specutils.analysis functions and the shared fit_lines implementation, then compare their current handling with extract_region and arithmetic, which already carry masks through. Determine how the mask should affect each analysis and fitting path, and resolve whether mask use is always-on or configurable. Done means the existing analysis and fitting tools consistently respect Spectrum1D masks.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.