Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

excise_regions() is not inclusive on the first boundary #1167

Closed
etsmit opened this issue Sep 17, 2024 · 2 comments · Fixed by #1171
Closed

excise_regions() is not inclusive on the first boundary #1167

etsmit opened this issue Sep 17, 2024 · 2 comments · Fixed by #1171
Labels
discussion manipulation Spectral manipulation tools
Milestone

Comments

@etsmit
Copy link

etsmit commented Sep 17, 2024

I am one of the developers for dysh, a project using specutils to perfom single-dish radio telescope calibration and data reduction. Upon testing baseline subtraction, we came across a discrepancy caused by the fact that the specutils excise_regions() function (or, either linear_exciser or true_exciser) are not inclusive on one boundary of each exclusion region, leading to a single channel unintentionally getting through. There are more details/discussions in the dysh issue, but is summarized by the pre- and post- excision spectra below:

367334702-30c028fa-506b-4e75-8a42-7b8b3b93235d

367335288-6ed54659-6fcf-446e-aee9-3c6401175553

where channel 0* gets missed by the true_exciser() function and dramatically shows up as the large spike to the right. Although only a small difference, this has implications for baseline fitting unit tests where we are trying to compare accuracy to a previous software suite.

Note: There are technically 3 excision regions in this example, so an extra 3 channels are unintentionally included by the excision. For the first two regions, one can simply compensate by increasing the boundary length by 1 channel. However, how does one change the boundary to be lower than 0?

I am adding a small PR with the change to the boundary conditions.

  • This is lower sideband data, which means that the frequency axis is flipped - the lower channel is the second boundary in frequency space.
@rosteen
Copy link
Contributor

rosteen commented Sep 19, 2024

I'm going to have to think about this one for a bit (I'm currently deep into a rabbit hole thinking about refractive indices) - I'm assuming that this was coded this way to be consistent with general Python indexing, where the lower bound is inclusive and the upper bound is exclusive. But it should at least be well documented, and we should make sure similar operations in the package are all consistent.

My initial thought is that, rather than changing it to always have an inclusive upper bound like your PR, it would be reasonable to keep the current Python-like indexing, but add a inclusive_upper argument to the excise functions to enable cases like yours. Otherwise you would need to hack around this in your exclude_to_region function (perhaps by adding a slight offset to the upper bound here), which I understand isn't ideal.

@rosteen
Copy link
Contributor

rosteen commented Sep 19, 2024

Side note, looking at the excision functions led me to immediately realize that there's a bug with true_excise not handling subregions properly...I'll open a PR for that shortly.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
discussion manipulation Spectral manipulation tools
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants