Conversation
…dec_2024 # Conflicts: # pypeit/core/datacube.py
…= True. The mask that is returned should always be the extraction mask used.
…into kcwi_dec_2024 # Conflicts: # pypeit/core/skysub.py
profxj
left a comment
profxj
left a comment
There was a problem hiding this comment.
Great stuff!
Do we need new docs (i.e. .rst files) or tests?
If so, demand them of your chat-bot. :)
| # Store the shift in the RA and DEC offsets in degrees | ||
| ra_offsets[ff] += ra_shift | ||
| dec_offsets[ff] += dec_shift | ||
| image_phase=False |
There was a problem hiding this comment.
Indeed, this should be removed. The Gauss fit will only work for point sources.
| return ra_offsets, dec_offsets | ||
|
|
||
| def compute_weights(self): | ||
| def compute_weights(self, show_qa=False): |
| self.all_tilts, self.all_slits, self.all_align, self.all_dar, | ||
| self.ra_offsets, self.dec_offsets, | ||
| outfile=outfile, overwrite=self.overwrite, whitelight_range=wl_wvrng, | ||
| # if self.combine: |
| idx (int, optional): | ||
| Index of filename to be saved. Required if combine=False. | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
Parameters
and
Returns:
do not go together.
| outfile (str): | ||
| The output filename for the datacube. | ||
| output_dir (str): | ||
| The output directory to save the datacube. |
|
|
||
|
|
||
|
|
||
| def make_whitelight_fromcube_old(cube, bpmcube, wave=None, wavemin=None, wavemax=None): |
There was a problem hiding this comment.
Maybe send to deprecate? Really, I think we should include this old functionality by not enforcing the sigma clipping.
|
|
||
|
|
||
|
|
||
| #extract_good_frac=0.005 |
| # Using the arxiv arc wavelength solution, search for the nearest line in the line list | ||
| bstwv = np.abs(wvdata - wvval_arxiv[bstpx]) | ||
| # This is a good wavelength match if it is within match_toler disperion elements | ||
| # This is a good wavelength match if it is within match_toler daisperion elements |
rcooke-ast
left a comment
rcooke-ast
left a comment
There was a problem hiding this comment.
Thanks, @jhennawi, for this massive revision of the datacube code. A lot of excellent updates here. I've been through the code carefully and commented extensively. Most of the changes are pretty straight forward, but I've marked some places where we should really discuss things on Slack (or here) before further. Once you've addressed these, please post a report of the tests. Thanks again!
| elif len(shape) == 3: | ||
| weights_stack = np.einsum('ij,k->ijk', weights, np.ones(shape[2])) | ||
| elif weights.ndim == 3: | ||
| elif (weights.ndim == 3) | (weights.ndim == 4): |
There was a problem hiding this comment.
I think the | (a bitwise operator) should be or (a logical operator).
|
|
||
| Args: | ||
| spatx (`numpy.ndarray`_): Array of spatial positions to hand extract | ||
| spaty (`numpy.ndarray`_): Array of spectral positions to hand extract |
There was a problem hiding this comment.
I think spectral should be spatial here. Perhaps make it clear which directions spatx and spaty corresponding to (presumably one aligns with either the spatial slice, or with declination)?
| Args: | ||
| spatx (`numpy.ndarray`_): Array of spatial positions to hand extract | ||
| spaty (`numpy.ndarray`_): Array of spectral positions to hand extract | ||
| fwhm (`numpy.ndarray`_): Array of FWHM for hand extraction in arcserconds. |
There was a problem hiding this comment.
Should this be "optional" as well, because in the datamodel below you list it as optional?
| idict['spatx'] += [float(parse[0])] | ||
| idict['spaty'] += [float(parse[1])] | ||
|
|
||
| # FWHM? |
There was a problem hiding this comment.
Should we be concerned that FWHM must be specified if you want to specify boxcar_rad? What if someone just wants the boxcar, for example?
| """ | ||
| if len(self.spatx) != len(self.spaty): | ||
| raise ValueError("spatx and spaty not of the same length") | ||
| #if len(self.fwhm) != len(self.spatx): |
There was a problem hiding this comment.
Should we also be checking FWHM and boxcar_rad? Seems to make sense.
| datacube generated from the subpixellated inputs, (2) the corresponding | ||
| variance cube, and (3) the corresponding bad pixel mask cube. | ||
| standard deviation cube, and (3) the corresponding bad pixel mask cube. (4) A cube | ||
| indicating the occupation number of a given pixel TODO: elaborate on this |
There was a problem hiding this comment.
elaborate here too, thanks.
| # Loop over the subslices | ||
| for ss in range(slice_subpixel): | ||
| if slice_subpixel > 1: | ||
| if verbose and slice_subpixel > 1: |
There was a problem hiding this comment.
for clarity, can we change this to: verbose and (slice_subpixel > 1)
| #this_dec_int += dec_corr + _dec_offset[fr] | ||
| # TODO: Below was a hack to fix bug for KCRM. I suspected the WCS was being set incorrectly, | ||
| # which was true, and this hack fixed it. Old code is the line above. | ||
| this_dec_int += dec_corr - _dec_offset[fr] |
There was a problem hiding this comment.
I'm not so sure about this change. Let's investigate this further. You have used a different/new approach to determine dec_offset, and we need to make sure that this is consistent with the old approach. The new method (in run_align()) may have a bug, and this line of code should be reverted to the old code. Please read the following section of the documentation to make sure that your new approach is consistent with the docs:
Setting offsets in PypeIt
| flxcube *= nc_inverse | ||
| varcube *= nc_inverse**2 | ||
| bpmcube = (normcube == 0).astype(np.uint8) | ||
| bpmcube = (normcube == 0) #.astype(np.uint8) |
There was a problem hiding this comment.
I think we can remove the commented code, or include the type set to uint8.
|
|
||
|
|
||
|
|
||
| def make_whitelight_fromcube_old(cube, bpmcube, wave=None, wavemin=None, wavemax=None): |
There was a problem hiding this comment.
Maybe send to deprecate? Really, I think we should include this old functionality by not enforcing the sigma clipping.
…aces are at the edge of the slit.
…aces are at the edge of the slit.
…dec_2024 # Conflicts: # doc/help/run_pypeit.rst # doc/include/dependencies_table.rst # pyproject.toml
Merges release and develop into #1929
4 participants
This PR involves major upgrades to KCWI and specifically KCRM functionality.