XSpectrum1D: sort wavelengths #125
Conversation
| if sig is not None: | ||
| sig = sig[srt] | ||
| xspec1d = XSpectrum1D.from_array(uwave, u.Quantity(fx), |
There was a problem hiding this comment.
Seems to do the job.
Suggestion: Wouldn't it be better to avoid code duplication and do the sorting only once? How about using a fixed naming convention for all the important attributes/parameters/arrays needed for defining XSpectrum1D and do the sorting and calling only once in the very latest stage?
There was a problem hiding this comment.
Yes, I wish. Unfortunately there are too many ways
to instantiate and I can't do anything about the WCS ones..
There was a problem hiding this comment.
It might be better to overload from_array so that it wraps Spectrum1D.from_array, but adds a keyword option sort which is false by default. What do you think?
|
The changes related to AbsComponent tests look all good to me. Feel free to merge unless Neil says otherwise... |
| rnd = np.random.rand() | ||
| else: | ||
| rnd = 0. | ||
| iline.attrib['logN'] = 13.3 + rnd |
There was a problem hiding this comment.
I like this change - I hate using random numbers in tests.
|
See my inline comment about maybe putting a sort option into |
|
I've modified from_tuple. Could probably eliminate the changes from io.py if I decided over-loading from_array was too dangerous.. |
|
Yes, let's use from_tuple in io.py to minimise changes there. Also can you make the sorting in from_tuple optional using a keyword? So it can be turned off for speed, if necessary. |
|
Done. |
|
Just merged with the continuum fitting PR. |
|
Should this PR be closed then? |
|
Please merge it On December 26, 2015 2:39:04 PM PST, Nicolas Tejos notifications@github.com wrote:
Sent from my Android device with K-9 Mail. Please excuse my brevity. |
|
Merging. |
and related arrays.
Fixes an issue with COS spectra.
Probably still not ideal