allow matched pixels/wavelengths to be in either direction #177
Conversation
Codecov Report
@@ Coverage Diff @@
## main #177 +/- ##
==========================================
+ Coverage 81.28% 81.35% +0.06%
==========================================
Files 10 10
Lines 983 992 +9
==========================================
+ Hits 799 807 +8
- Misses 184 185 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>
|
Can we remove the internal sorting with this check in place? Is there anything fundamental in the logic that would prevent us from handling the inverted case (besides the internal sorting - if the check was instead to make sure that they either increase or decrease, would that work)? |
cshanahan1
changed the title
| if not _check_arr_monotonic(line_wavelengths): | ||
| if str(line_wavelengths.unit.physical_type) == "frequency": | ||
| raise ValueError('Frequencies must be strictly increasing or decreasing.') | ||
| raise ValueError('Wavelengths must be strictly increasing or decreasing.') |
There was a problem hiding this comment.
Not for this PR, but I wonder if we should rename line_wavlengths to line_spectral_values or something?
There was a problem hiding this comment.
yeah, also we should not be checking strictly for a 'wavelength' column if frequencies are allowed.
| if not _check_arr_monotonic(line_wavelengths['wavelength']): | ||
| raise ValueError('Wavelengths must be strictly increasing or decreasing.') |
There was a problem hiding this comment.
do we support a frequency option for the QTable case?
There was a problem hiding this comment.
thats what was implied, but are checks for the presence of a 'wavelength' column so it seems like you could input something with frequency units but the column must be named wavelength which is confusing.
i also am not a fan of all the different input options available - i feel like it would be a lot less confusing if we just forced users to input a table with pixel and wavelength or frequency (or eventually, the catalog option) instead of allowing two matched arrays, or a table, or a table with a pixel column and an array, or two tables each with one col. i think this is too many options and a lot of this code is just figuring out the logic of the inputs where we could just insist on users constructing a table beforehand (or, and i might like this option better because then youre not checking for table column names, an input pixel array and an input wavelength/freq array, and then internally convert to a table).
|
thanks everyone, im going to merge this now :) |
This PR removes sorting in wavelength calibration to allow for either right-to-left or left-to-right inputs. A check that input matched pixels/wavelengths are monotonic in either direction is added.
Addresses #174