Add resampling extension

[icweaver]

Closes: #27

Depends on: #24

Usage

julia> using Spectra: spectrum, flux, wave, SpectrumResampler

julia> using DataInterpolations: LinearInterpolation, ExtrapolationType

julia> spec = spectrum([20, 40, 120, 160, 200], [1, 3, 7, 6, 20]);

julia> interp = LinearInterpolation(flux(spec), wave(spec);
           extrapolation = ExtrapolationType.Constant
       );

julia> resampler = SpectrumResampler(spec, interp);

julia> wave_sampled = [10, 50, 90, 130, 140, 170, 210, 220, 230];

julia> result = resampler(wave_sampled);

Design questions

• Do we want an exported resample/resample! with a pre-baked interpolator so that things like resample(spec, new_wave) work OOTB? This would add an interpolation dep
• Would it be useful to add a method so that things like result.flux work, or is it preferable to stick with public getters like Spectra.flux(result::SpectrumResampler)?

To-do

• docs
• tests
• use accessor methods more
• update things once Unify XSpectrum types #24 is in. Should work on all XSpectrums

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.74%. Comparing base (5c7e32d) to head (fab8c54).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   90.00%   90.74%   +0.74%     
==========================================
  Files           7        8       +1     
  Lines         150      162      +12     
==========================================
+ Hits          135      147      +12     
  Misses         15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cgarling]

Requiring construction of the interpolator prior to using resample can be performant if you need to perform many resampling operations but often you just want to do it once so it might be a nuisance to pre-construct the interpolator. I added a method that internally constructs the interpolator so the user just has to specify what kind of interpolation they want and and kws... they want to control the extrapolation.

Let me know what you think

[icweaver]

Thanks! I like how we give users the choice to go either route now. I added Interpolations.jl as another interpolator extension to be more in line with what PhotometricFilters.jl uses, and to start playing around with what a uniform interface might look like between these two extensions. Things are a bit fiddly right now, but I think it's starting to go in the right direction. What do you think about this current design? 6426edb

[cgarling]

I'm actually reconsidering the method that automatically constructs the interpolator; now that I see how it's inconvenient for Interpolations.jl (because of how the interpolation types are set up there), I don't like it as much.

I wonder if we can do this without the need to write the extensions. I added a SpectrumResampler type that bundles a Spectrum with an arbitrary interpolator. I think maybe this will "just work" for any interpolator type? I also like that it encapsulates both the spectrum and the interpolator so you don't have to do bookkeeping to keep track of a spectrum and its interpolator separately.

I made it so when it's called to resample the underlying spectrum, it returns a new SpectrumResampler instance that contains the new spectrum, and the original interpolator so you could, for example, resample the same spectrum in a loop like

spec = SpectrumResampler(...)
for i in 1:N
    some_spectrum = <load spectrum from file>
    spec = spec(wave(some_spectrum)) # Resample `spec` at new wavelengths in `some_spectrum`
    <do stuff>
end

the wave(spec) and flux(spec) will change, but spec.interp will always be the same and will not have to be recomputed.

Probably we should make SpectrumResampler a subtype of AbstractSpectrum and give it the same indexing methods and everything as base Spectrum, but I haven't done that yet as this is just proof of concept.

I'm also not committed to whether it should just return a spectrum or a SpectrumResampler but generally this seems like it might be a better solution so we don't have to write extensions.

[icweaver]

Hi Chris, sg! I like the bundling of the resampled spectra and its interpolator.

If folks are cool with just bringing their own interpolator, then that def makes things simpler and no extensions are needed. I just made them because I wasn't sure how lazy spectroscopists were =p. If they're cool with bringing their own interpolator, then this sounds like a win all around in terms of maintainability and expressiveness. Will start filling things out with this new design and start playing around with the return types

[cgarling]

My design is sort of similar to that of specutils (see here) but they require the use of their own interpolation methods where we can, hopefully, support interpolations provided by any other package. We should provide examples with specific interpolation methods for clarity though

[icweaver]

Ok, got an example going here. I decided to drop resample to hopefully avoid confusion with how similar it is to SpectrumResampler

[icweaver]

For the indexing, I think I want to try exploring #24 a bit more, and hopefully get that in first if we land on a design that seems ok

[cgarling]

Thanks for adding this! A small nitpick but it seems to me like we are duplicating examples in the docs file and in the jldoctest for the SpectrumResampler. Ideally these examples would only appear in one place (avoids scenarios where the two examples may become out of sync if one is updated and the other is not).

My preference is to place all examples possible in a jldoctest on the actual docstring. This enables you to see the examples when you query the docstring from the REPL, and you can run the doctests as part of the test suite so that the doctests are counted in the test coverage. Then we just include the docstring in the documentation page.

I expanded the examples in the docstring to cover both Interpolations.jl and DataInterpolations.jl. My preference would be to replace the @repl blocks with a sufficiently detailed docstring for the SpectrumResampler.

I think blocks like @repl and @example are best used when you want to show something that can't be well-represented in the text-only docstring. A good example would be if we wanted to compare how different interpolation algorithms deal with sharp lines in spectra -- we could plot the original spectrum and several different interpolations to compare them.

This is partially a style choice so let me know what you think!

[cgarling]

For now using Spectrum() rather than spectrum() in the SpectrumResampler saves some allocations but I think it's due to how the meta dict is being handled. If we change how we deal with the metadata (e.g,. by using a NamedTuple rather than a dict) this may change but just making a note for now

[icweaver]

Makes sense, thanks!

Added this note to the unification re-work

[icweaver]

Cool, things are looking green so far. I added a to-do in the parent comment to make sure things still work alright with #24

[icweaver]

After playing around with the spectrum unification PR a bit, I think I am leaning back towards SpectrumResampler just returning a regular Spectrum object 5b15e7b

This would reduce the number of additional methods we need to define when doing algebra between spectra, for example, and also looks to be the same design decision that specutils went with.

[cgarling]

I like that

[icweaver]

Ok, looking all green here. I guess the next step will be to merge in #24 after we wrap it up

[icweaver]

Yep, merging this to prep for Fergus' X-ray build out