JP-3755: Remove unused options and add unit tests to SOSS extraction algorithm #9000
Conversation
…tests for that function
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9000 +/- ##
==========================================
+ Coverage 76.81% 78.28% +1.47%
==========================================
Files 496 504 +8
Lines 45610 46039 +429
==========================================
+ Hits 35034 36042 +1008
+ Misses 10576 9997 -579
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| wave_grid : (N_k) array_like, required. | ||
| The grid on which f(lambda) will be projected. | ||
| mask_trace_profile : (N_ord, N, M) list or array of 2-D arrays[bool], required. | ||
| A list or array of the pixel that need to be used for extraction, | ||
| for each order on the detector. It has to have the same (N_ord, N, M) as `trace_profile`. |
There was a problem hiding this comment.
Made these two parameters required because they were always specified explicitly when the ExtractionEngine was instantiated.
| raise ValueError(msg.format(self.n_orders, len(wave_map))) | ||
|
|
||
| # Detector image. | ||
| self.data = np.full(self.data_shape, fill_value=np.nan) |
There was a problem hiding this comment.
having attributes self.data and self.error is unnecessary. The ExtractionEngine does not itself need any data to set up a model of the detector and use most of the functionality. The only functions where these were accessed had optional data and error parameters, and in reality these were always set non-trivially when calling the pipeline. These were made required for those specific functions, specifically get_detector_model(), get_pixel_mapping(), and compute_likelihood(). estimate_noise() was unused in the pipeline and removed
| # Set all reference file quantities to None. | ||
| self.wave_map = None | ||
| self.trace_profile = None | ||
| self.throughput = None | ||
| self.kernels = None | ||
|
|
||
| # Set the wavelength map and trace_profile for each order. | ||
| self.update_wave_map(wave_map) | ||
| self.update_trace_profile(trace_profile) |
There was a problem hiding this comment.
These are all set to something other than None later in the __init__, so there was no point setting them to None. The update_wave_map() function was removed because this was the only place it was called, and it could be replaced with a one-liner. The same is true for update_trace_profile(): its use in get_detector_model() turned out to also be unnecessary because the option to pass trace_profile into get_detector_model() was never used in the pipeline.
| self.update_trace_profile(trace_profile) | ||
|
|
||
| # Set the mask based on trace profiles and save | ||
| if mask_trace_profile is None: |
There was a problem hiding this comment.
mask_trace_profile was never set to None in real calls to this class, so I made it required and removed the option to set it to None
| self.mask_trace_profile = mask_trace_profile | ||
|
|
||
| # Generate a wavelength grid if none was provided. | ||
| if wave_grid is None: |
There was a problem hiding this comment.
wave_grid was never set to None in real calls to this class, so I made it required and removed the option to set it to None
| return bkg_mask | ||
|
|
||
|
|
||
| def soss_oneoverf_correction(scidata, scimask, deepstack, bkg_mask=None, |
| log.setLevel(logging.DEBUG) | ||
|
|
||
|
|
||
| def zero_roll(a, shift): |
| return result | ||
|
|
||
|
|
||
| def robust_polyfit(x, y, order, maxiter=5, nstd=3.): |
There was a problem hiding this comment.
only used as a helper to unused code in soss_centroids
| return param | ||
|
|
||
|
|
||
| def get_image_dim(image, header=None): |
There was a problem hiding this comment.
only used as a helper to unused code in soss_centroids
| # In both orders, the errors on the replaced pixels are roughly | ||
| # half of the errors on the original good pixels | ||
| # There are enough replaced pixels here (~30) that small-number statistics cannot account for this | ||
| # The reason is because the code chooses the lower error between the two nearest-flux | ||
| # data points, and since the errors in our tests are uncorrelated with the flux values, | ||
| # this leads to a factor-of-2 decrease | ||
| # It's not clear to me that picking the smaller of two error values is the right thing to do | ||
| # but that behavior is documented |
There was a problem hiding this comment.
Wondering if this is actually the intended behavior or not, because it seems like a questionable choice to me (although admittedly I don't know very much).
Resolves JP-3755
Closes #8804
This PR addresses reducing the technical debt of the SOSS ATOCA algorithm. There are a lot of changed lines, so I'm annotating this PR with comments during a self-review. The high-level changes are:
There are several outstanding items to complete before this can be merged:
Tasks
Build 11.3(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.datamodels.rstchanges/<PR#>.scripts.rstchanges/<PR#>.fits_generator.rstchanges/<PR#>.set_telescope_pointing.rstchanges/<PR#>.pipeline.rststage 1
changes/<PR#>.group_scale.rstchanges/<PR#>.dq_init.rstchanges/<PR#>.emicorr.rstchanges/<PR#>.saturation.rstchanges/<PR#>.ipc.rstchanges/<PR#>.firstframe.rstchanges/<PR#>.lastframe.rstchanges/<PR#>.reset.rstchanges/<PR#>.superbias.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.rscd.rstchanges/<PR#>.persistence.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.charge_migration.rstchanges/<PR#>.jump.rstchanges/<PR#>.clean_flicker_noise.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.gain_scale.rststage 2
changes/<PR#>.assign_wcs.rstchanges/<PR#>.badpix_selfcal.rstchanges/<PR#>.msaflagopen.rstchanges/<PR#>.nsclean.rstchanges/<PR#>.imprint.rstchanges/<PR#>.background.rstchanges/<PR#>.extract_2d.rstchanges/<PR#>.master_background.rstchanges/<PR#>.wavecorr.rstchanges/<PR#>.srctype.rstchanges/<PR#>.straylight.rstchanges/<PR#>.wfss_contam.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.fringe.rstchanges/<PR#>.pathloss.rstchanges/<PR#>.barshadow.rstchanges/<PR#>.photom.rstchanges/<PR#>.pixel_replace.rstchanges/<PR#>.resample_spec.rstchanges/<PR#>.residual_fringe.rstchanges/<PR#>.cube_build.rstchanges/<PR#>.extract_1d.rstchanges/<PR#>.resample.rststage 3
changes/<PR#>.assign_mtwcs.rstchanges/<PR#>.mrs_imatch.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.exp_to_source.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.tso_photometry.rstchanges/<PR#>.stack_refs.rstchanges/<PR#>.align_refs.rstchanges/<PR#>.klip.rstchanges/<PR#>.spectral_leak.rstchanges/<PR#>.source_catalog.rstchanges/<PR#>.combine_1d.rstchanges/<PR#>.ami.rstother
changes/<PR#>.wfs_combine.rstchanges/<PR#>.white_light.rstchanges/<PR#>.cube_skymatch.rstchanges/<PR#>.engdb_tools.rstchanges/<PR#>.guider_cds.rst