Template redshift enhancements #551
Conversation
Update master in fork
| The maximum redshift allowed. | ||
| delta_redshift : `float` | ||
| The amount the redshift will change between loops. | ||
| redshift : `list`, `tuple`, 'numpy.array` |
There was a problem hiding this comment.
I think we should merge redshift and known_redshift into a single parameter. We can then check if the value is a float or array-like and deal with it appropriately. Using this function, I wouldn't expect that a single value or a list of values would require two different arguments.
There was a problem hiding this comment.
I agree with this, but the one question is: is there some specific science case for known_redshift being used in addition to the redshift grid? I can't think of one offhand but just want to be sure... maybe @camipacifici has some insight?
| min_redshift, max_redshift, delta_redshift) | ||
| if redshift is not None and all(x is not None for x in redshift): | ||
| _, redshifted_spectrum, _ = template_redshift(observed_spectrum, spectral_templates, | ||
| redshift=redshift) | ||
| spectral_templates = redshifted_spectrum | ||
| elif known_redshift: |
There was a problem hiding this comment.
(This elif block can then be removed if we go with a single redshift argument that can be either a float or a list.)
| min_redshift, max_redshift, delta_redshift) | ||
| if redshift is not None and all(x is not None for x in redshift): | ||
| _, redshifted_spectrum, _ = template_redshift(observed_spectrum, spectrum, | ||
| redshift=redshift) | ||
| spectrum = redshifted_spectrum | ||
|
|
||
| elif known_redshift: |
| The maximum redshift allowed. | ||
| delta_redshift : `float` | ||
| The amount the redshift will change between loops. | ||
| redshift : `list`, `tuple`, 'numpy.array` |
There was a problem hiding this comment.
| redshift : `list`, `tuple`, 'numpy.array` | |
| redshift : `float`, `list`, `tuple`, 'numpy.array` |
| raise ValueError("The `max_redshift` value must be greater than `min_redshift`.") | ||
| return | ||
|
|
||
| redshift = min_redshift | ||
| chi2_min = None |
There was a problem hiding this comment.
| chi2_min = None | |
| redshift = np.array(redshift).reshape((np.array(redshift).size,)) | |
| chi2_min = None |
| The maximum redshift allowed. | ||
| delta_redshift : `float` | ||
| The amount the redshift will change between loops. | ||
| redshift : `list`, `tuple`, 'numpy.array` |
There was a problem hiding this comment.
I agree with this, but the one question is: is there some specific science case for known_redshift being used in addition to the redshift grid? I can't think of one offhand but just want to be sure... maybe @camipacifici has some insight?
| @@ -224,29 +223,28 @@ def template_redshift(observed_spectrum, template_spectrum, min_redshift, max_re | |||
| redshifted_spectrum: :class:`~specutils.Spectrum1D` | |||
| A new Spectrum1D object which incorporates the template_spectrum with a spectral_axis | |||
| that has been redshifted using the final_redshift. | |||
| chi2_list : `list` | |||
There was a problem hiding this comment.
I think this should be an ndarray instead of a list. That will make it a lot easier to work with if it's multidimensional.
Relatedly, I'm having a bit of trouble following the logic: is the resulting array is of dimension equal to the spectral template set, or is it a 2D array that's n_template x n_redshift ? I think the latter is the eventual goal, but if we're not quite there it could be a follow-on.
There was a problem hiding this comment.
In some out-of-band discussion, @ibusko said he'd write a test for the SpectrumCollection case and see what happens.
My preference is that it returns a 2D array as indicated above (or the transpose)
This PR addresses issue #549 .