specutils excise_regions is not inclusive on the first boundary

[etsmit]

When developing a numpy baseline routine for issue 86 I realized that the first boundary of a given excise region was skipped.

import specutils.manipulation.utils as smu

from dysh.fits.gbtfitsload import GBTFITSLoad
from dysh.spectra.core import exclude_to_region

import dysh

import matplotlib.pyplot as plt

fname = 'AGBT17A_404_01_scan_19_prebaseline.fits'

sdf = dysh.fits.gbtfitsload.GBTFITSLoad(fname)
spec = sdf.getspec(0)
spec.plot()

# remove bandpass edges and possible signal
ex_reg = [(0,100),(381,450),(721,819)]

rl = exclude_to_region(ex_reg,spec)
spec = smu.excise_regions(spec, rl)

#can't use dysh plot function - 
# -likely not a real issue since excise_regions isn't used directly by the user like this
plt.plot(spec.frequency, spec.data)

This is lower sideband data, so channel 0 is the highest frequency (right side). You can see that the plot removes the left side (doesn't plot), skips over the signal in the middle, and removes most of the right side except for channel 0, which is marked by the straight line going up. This obviously doesn't have much significance as far as baseline fitting, but it is an oddity that should be addressed.

Obviously, putting -1 as the left boundary returns an error, but using the frequencies also returns the same behavior. For example, putting the left boundary as 1.40168216 GHz (the furthest left channel) returns an out-of-bounds error, and dropping the frequency doesn't excise that channel.

Along that thought, I would like to request that region boundaries that are out-of-bounds just snap to the edge of the spectrum (with a warning) instead of returning the out-of-bounds error. It feels kind of silly to put a valid frequency value that is the edge of the band and to just have it error out.

Also, these are all in specutils, so it's unclear what we can do from the dysh side.

[astrofle]

Great work. Since the issue is with specutils this seems like a good argument in favor of moving away from it for the baseline fitting.

Could you share the location of the data you're using to reproduce and investigate the issue. We should also submit an issue to specutils (even if we decide to not use it for this).

Regarding the specutils behaviour, Could this be it?

region_mask = (spectral_axis >= region.lower) & (spectral_axis < region.upper)

from https://github.com/astropy/specutils/blob/173d80606c2e60d759a77a946dc49d0fb382f738/specutils/manipulation/utils.py#L46C9-L46C87

[etsmit]

The data is in testdata/AGBT17A_404_01/.

specutils does use "spectral regions" to denote the excised parts, which understands different units (Hz, channels, GHz etc.) so if we move totally away from specutils we will likely have to handle that kind of unit conversion in-house. Not impossible but it is an extra step

[etsmit]

I'm able to manipulate my local specutils install to be inclusive on both ends of each excision region (It was as simple as replacing a < with a <= in manipulation.utils.true_exciser). I don't know how worth it it is to PR specutils with this, since it seems like a big change to how excision works for everyone else that uses specutils.

It also correctly uses the true_exciser, rather than the linear exciser. I am going to test the baseline subtractions again, and see if this is the only thing that needs to change to fix issue 86.

[astrofle]

@etsmit does the "fix_exclude" option of baseline do what you want in terms of snapping to the edges of the spectral axis? The option is kind of hidden though.

From the exclude_to_region documentation:

fix_exclude: bool
        If True, fix exclude regions that are out of bounds of the specctral axis to be within the spectral axis. Default:False

[rosteen]

I don't want to change the default behavior in specutils, which is to be consistent with generic python slicing, but astropy/specutils#1171 adds an optional keyword to toggle on inclusive upper bounds in fitting and excision. Is that sufficient to meet your needs here?

Since the issue is with specutils this seems like a good argument in favor of moving away from it for the baseline fitting.

Just wanted to say that I appreciate that you all took the step of reaching out rather than just quietly dropping specutils! The benefits of open source development 🙂

[astrofle]

I don't want to change the default behavior in specutils, which is to be consistent with generic python slicing, but astropy/specutils#1171 adds an optional keyword to toggle on inclusive upper bounds in fitting and excision. Is that sufficient to meet your needs here?

Since the issue is with specutils this seems like a good argument in favor of moving away from it for the baseline fitting.

Just wanted to say that I appreciate that you all took the step of reaching out rather than just quietly dropping specutils! The benefits of open source development 🙂

@rosteen Yes, that would solve our issue. Thanks a lot!

[rosteen]

@rosteen Yes, that would solve our issue. Thanks a lot!

Great - I just merged that update into main, I'll release 1.17.0 with that and a couple other things probably midweek.

[etsmit]

dysh update in PR 403