Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix excisor bounds #403

Merged
merged 5 commits into from
Nov 28, 2018
Merged

Fix excisor bounds #403

merged 5 commits into from
Nov 28, 2018

Conversation

nmearl
Copy link
Contributor

@nmearl nmearl commented Nov 27, 2018

Minor fix to bounds in excised region.

Example of bug:

In [23]: from specutils import SpectralRegion

In [24]: from specutils.fitting import fit_generic_continuum

In [25]: spec = Spectrum1D(flux=np.random.sample(100) * u.Jy, spectral_axis=np.arange(100) * u.AA)

In [26]: inc_regs = SpectralRegion(80 * u.AA, 90 * u.AA) + SpectralRegion(50 * u.AA, 60 * u.AA)

In [27]: exc_regs = inc_regs.invert_from_spectrum(spec)

In [28]: cont_mod = fit_generic_continuum(spec, exclude_regions=exc_regs)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-28-a7adc8495e39> in <module>()
----> 1 cont_mod = fit_generic_continuum(spec, exclude_regions=exc_regs)

~/projects/specutils/specutils/fitting/continuum.py in fit_generic_continuum(spectrum, median_window, model, fitter, exclude_regions, weights)
     55     #
     56
---> 57     return fit_continuum(spectrum_smoothed, model, fitter, exclude_regions, weights)
     58
     59

~/projects/specutils/specutils/fitting/continuum.py in fit_continuum(spectrum, model, fitter, exclude_regions, window, weights)
     95     #
     96
---> 97     continuum_spectrum = fit_lines(spectrum, model, fitter, exclude_regions, weights)
     98
     99     return continuum_spectrum

~/projects/specutils/specutils/fitting/fitmodels.py in fit_lines(spectrum, model, fitter, exclude_regions, weights, window, **kwargs)
     64
     65     if exclude_regions is not None:
---> 66         spectrum = excise_regions(spectrum, exclude_regions)
     67
     68     #

~/projects/specutils/specutils/manipulation/utils.py in excise_regions(spectrum, regions, exciser)
    102
    103     for region in regions:
--> 104         spectrum = excise_region(spectrum, region, exciser)
    105
    106     return spectrum

~/projects/specutils/specutils/manipulation/utils.py in excise_region(spectrum, region, exciser)
    150     #
    151
--> 152     return exciser(spectrum, region)

~/projects/specutils/specutils/manipulation/utils.py in linear_exciser(spectrum, region)
     52     flux = spectrum.flux.value
     53     modified_flux = flux
---> 54     modified_flux[s:e] = np.linspace(flux[s], flux[e], len(inclusive_indices)+1)
     55
     56     # Return a new object with the regions excised.

ValueError: could not broadcast input array from shape (51) into shape (0)

In [29]:

@nmearl nmearl requested a review from eteq November 28, 2018 20:32
Copy link

@brechmos-stsci brechmos-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nmearl nmearl merged commit 7653656 into astropy:master Nov 28, 2018
@eteq
Copy link
Member

eteq commented Nov 29, 2018

Not a big deal, but minor suggestions for this sort of thing in the future: since the test is specifically for a known bug it's good to reference the issue for future devs. E.g. a comment saying "this is a test for the issue reported in #403), or even call the function test_regression_403. Not worth changing this after the fact, but perhaps for future PRs, @nmearl

SaOgaz pushed a commit that referenced this pull request Mar 25, 2019
Update installation.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants