Template redshifting using chi2

[javerbukh]

This is still a hacky version of what will eventually become the template redshifting solution found in #488.
The current issue I'm having is with testing the redshift solution. My current test goes from a min_redshift of .5 to a max_redshift of 5.5, with a delta_redshift of .25. The test prints the current redshift along with the corresponding chi2. Everything seems to work as intended, except at a redshift of 1, where the chi2 is exactly the same as the intended correct redshift of 3. @camipacifici if you could help me understand this result that would be much appreciated!

Resolves #488

[javerbukh]

What should happen in the code when the spectral axes of the observed and template spectra do not overlap? Currently a nan is returned. Is this correct or should something else happen?
@camipacifici @nmearl @eteq

[camipacifici]

Do not overlap entirely or partially?
I think that the code should send an error/warning saying that at the "attempted" redshift, the template does not sample the observed wavelength range. I could imagine that this could happen because the "attempted" redshift is really far from reality, or the template spectrum does not cover a large enough wavelength range.

[nmearl]

Some minor quibbles.

[eteq]

I had some inline comments and also some bigger-picture comments here:

• While I see the motivation for the min_redshift/max_redshift/delta_redshift approach, after thinking about it a bit I think I favor instead having the user pass in a single redshifts keyword. My reasoning is that I know there are times I want to pass in a custom grid - e.g. logarithmic, or if I know I want a particular area of redshift to focus on but also have a broad grid. E.g. to capture "it's probably in a particular redshift where I think it is based on some other external dataset, but maybe it's an outlier and actually at a very different redshift". So given that requirement, I'd say just have a single redshifts keyword, but leave the min/max/delta in the documentation by doing np.arange(min_redshift, max_redshift, delta_redshift) and pass the result of that in as the redshift grid. That has the added advantage that if we later want to add a redshift fitting techinique (instead of this brute-force grid approach), we can pass that in via this keyword.
• Also, I think it would be useful if these functions return the chi-squared of the other templates at their best-fit redshift, not just the best one. The reason why is that in the science cases I know for this you often have several templates, and sometimes it's not that clear which is the best one. So it's useful to see if there are several templates with roughly similar chi-squared, indicating that several are possible matches, or rather instead it's that there's one that's clearly better.
• This was really an oversight in the previous PR I think, but the specutils/analysis/template_comparison.py should have an __all__ attribute which lists the functions that are actually supposed to be used by the user (and the function added here should be in it along with template_match). See https://github.com/astropy/specutils/blob/master/specutils/analysis/uncertainty.py#L10 for an example if you're not sure what I mean by this.

[eteq]

probably this should also return the best-fit chi-square (or perhaps all of the chi-squareds? see my general comment on this)

[camipacifici]

@eteq, you raised very valid big-picture points. I agree that these features will be very useful, but I would suggest to finish this as it was originally designed for now and treat your points as enhancements that will be implemented in the near future.
For example, I would envision returning the chi-square values for all models not just when changing the redshift, but also for the different templates in case the user wants to build probability density functions for other parameters associated with the templates.

Would you be ok with this?

[nmearl]

This looks good. There's only one actual concern (the rest are minor grammatical suggestions), and that is the verbiage of the doc string from the template comparison. I'm not quite why this is such a distinction: if the user knows the redshift and their spectral templates aren't at that redshift, why not allow the user to provide the known redshift and have the function do the appropriate shifting? Technically, this seems already possible, yeah? Given a redshift of 0.2, for instance, I could pass in my un-redshifted templates to the function with min_redshift=0.2, max_reshift=0.2, delta_redshift=0. But that seems silly -- instead, we could offer a redshift parameter that just applies that redshift to each template before attempting the match.

[nmearl]

This looks good to me as is. I think another issue ought to be opened with @eteq's suggestions.

[eteq]

Unfortunately it's too late if it just got merged, but I was not ok with merging this as-is. My first two bullets are about permanent API decisions we are stuck with if we do a release with this PR. That's why I marked it as "change requested" as opposed to "accept"ing the PR and making the suggestions as-is. (The third one looks to have been implemented though based on the diff, so that's good!)

They are relatively straightforward changes though, so perhaps the best path forward is to make implementing these (or agreeing that we do not want to implement them) a high-priority by treating them as blockers for the next specutils release?

[eteq]

To say a bit more on the above - the reason to resist merging it is that we don't want to let it drop and accidentally do a release having not finished it off. An alternate workflow that would have been find would have been merging but making sure there was an issue created before (or at the time of merging) tagged for the current/upcoming release that serves as the reminder to either make those changes or agree we aren't going to. (that makes it a blocker for release so that we don't just let it drop/forget about it)

[hcferguson]

@eteq Agreed on creating the issue(s). Not sure why you see it as a blocker for the next release as opposed to a v1.0 (API stable) release, but that does seem like a good approach to making sure it doesn't get dropped.

I'm a bit worried that the elaboration of the features and API is now so fragmented between issues and PRs that it's hard to follow (although maybe that's just me)? Should someone put together a consolidated description of what we are aiming at? Including a (updated?) py-in-the-sky notebook with use cases?

[javerbukh]

@eteq I opened up an issue with your points: #549. I'm not sure if you mentioned those points in a another issue or PR - or in a tag up - but that is the first time I heard those suggestions and it was when the PR was essentially finished according to the original ticket and according to all discussion leading up to this point. We can discuss those points further in future sprints and implement them as soon as possible.