Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New routine: ocrreject_exam() #169

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

m-dallas
Copy link

Checks STIS CCD 1D spectroscpopic data for cosmic ray overflagging. Behaves differently if plotly is installed.

stistools.ocrreject_exam.ocrreject_exam("odvkl1040", plot=True)

@m-dallas m-dallas requested a review from a team as a code owner December 10, 2024 21:53
@m-dallas m-dallas self-assigned this Dec 10, 2024
@m-dallas m-dallas requested review from sean-lockwood and removed request for a team December 10, 2024 21:54
@sean-lockwood
Copy link
Member

sean-lockwood commented Jan 7, 2025

@m-dallas -
Would you mind pulling in Master (from the spacetelescope version) to your branch to make sure the docs build ok? I'd be happy to help you figure out the git commands; just let me know.

@sean-lockwood
Copy link
Member

@m-dallas -
On second thought: Don't do that. We don't want to confuse your changes with those from the other PRs. I think rebasing is the best way, but will take a little learning to figure out how to do it right.

@sean-lockwood
Copy link
Member

Assuming your fork is called "origin", you can add the original spacetelescope repo as "upstream".

Check your remotes first:

git remote -vv

Make the changes:

git remote add upstream [email protected]:spacetelescope/stistools.git
git fetch upstream
git rebase upstream/master

Then you should be able to overwrite your local branch:

git push --force origin md_ocrreject_exam

@m-dallas m-dallas requested a review from a team January 8, 2025 20:01
@m-dallas
Copy link
Author

m-dallas commented Jan 8, 2025

Tested on plhstins4 and passed using python 3.13.1 with 2 warnings:

../../../user/mdallas/linux/miniconda3/envs/test_env/lib/python3.13/site-packages/stsci/tools/nmpfit.py:8
  /user/mdallas/linux/miniconda3/envs/test_env/lib/python3.13/site-packages/stsci/tools/nmpfit.py:8: UserWarning: NMPFIT is deprecated - stsci.tools v 3.5 is the last version to contain it.
    warnings.warn("NMPFIT is deprecated - stsci.tools v 3.5 is the last version to contain it.")

../../../user/mdallas/linux/miniconda3/envs/test_env/lib/python3.13/site-packages/stsci/tools/gfit.py:18
  /user/mdallas/linux/miniconda3/envs/test_env/lib/python3.13/site-packages/stsci/tools/gfit.py:18: UserWarning: GFIT is deprecated - stsci.tools v 3.4.12 is the last version to contain it.Use astropy.modeling instead.
    warnings.warn("GFIT is deprecated - stsci.tools v 3.4.12 is the last version to contain it."

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

@sean-lockwood
Copy link
Member

@m-dallas -

Did you intend for the resulting scalars to be of type np.float64 rather than just float?:

ocrreject_exam('odvkl1040', plot=False, verbose=True)

For odvkl1040
Average across all extraction boxes: 31.8%
Average across all external regions: 0.8%
Average ratio between the two: 37.49

[{'rootname': 'odvkl1040',
  'extr_fracs': array([0.31530762, 0.32006836]),
  'outside_fracs': array([0.00884673, 0.00810278]),
  'ratios': array([35.64113429, 39.50106762]),
  'avg_extr_frac': np.float64(0.31768798828125),
  'avg_outside_frac': np.float64(0.008474755474901575),
  'avg_ratio': np.float64(37.486389928547126)}]

@sean-lockwood
Copy link
Member

Manual checks of the logic to use matplotlib and plotly look good to me. Command line usage works well.

@sean-lockwood
Copy link
Member

We should probably make an explicit check for the relevant CCD mode to avoid help calls. I imagine users might run it on their whole visits. For instance, I (intentionally) ran your code on first-order MAMA data and got a cryptic error message:

KeyError: "Keyword 'CRSPLIT' not found."

I would check the following keywords:

  • INSTRUME == 'STIS'
  • DETECTOR == 'CCD'
  • OBSMODE == 'ACCUM' (not an acquisition file)
  • NEXTEND >= 6

And do this in a try/except so that a KeyError also triggers this check.

with fits.open(flt_file) as flt_hdul:
    try:
        instrument = flt_hdul[0].header['INSTRUME']
        detector   = flt_hdul[0].header['DETECTOR']
        obsmode    = flt_hdul[0].header['OBSMODE']
        nextend    = flt_hdul[0].header['NEXTEND']

        if (instrument.strip() != 'STIS') or \
           (detector.strip() != 'CCD')    or \
           (obsmode.strip() != 'ACCUM')   or \
           (nextend < 6):
            raise ValueError
    except (KeyError, ValueError,):
        raise ValueError(f"{flt_file}: Not a STIS/CCD/ACCUM file with ≥2 SCI extensions.")

    # Your code...
    propid = flt_hdul[0].header['PROPOSID']

@m-dallas
Copy link
Author

m-dallas commented Jan 9, 2025

@sean-lockwood -

Did you intend for the resulting scalars to be of type np.float64 rather than just float?:

ocrreject_exam('odvkl1040', plot=False, verbose=True)

For odvkl1040
Average across all extraction boxes: 31.8%
Average across all external regions: 0.8%
Average ratio between the two: 37.49

[{'rootname': 'odvkl1040',
  'extr_fracs': array([0.31530762, 0.32006836]),
  'outside_fracs': array([0.00884673, 0.00810278]),
  'ratios': array([35.64113429, 39.50106762]),
  'avg_extr_frac': np.float64(0.31768798828125),
  'avg_outside_frac': np.float64(0.008474755474901575),
  'avg_ratio': np.float64(37.486389928547126)}]

That wasn't intentional, just the default behavior of dividing an np.sum:
avg_extr_frac = (np.sum(extr_fracs))/(len(extr_fracs)) # Average fraction of crs inside extraction box

I changed the relevant lines to:
avg_extr_frac = float((np.sum(extr_fracs))/(len(extr_fracs))) # Average fraction of crs inside extraction box

for consistency

@m-dallas
Copy link
Author

m-dallas commented Jan 9, 2025

@sean-lockwood

We should probably make an explicit check for the relevant CCD mode to avoid help calls. I imagine users might run it on their whole visits. For instance, I (intentionally) ran your code on first-order MAMA data and got a cryptic error message:

KeyError: "Keyword 'CRSPLIT' not found."

Good point, I implemented your fix and it works as expected on the MAMA exposure in a list I gave it:

stistools.ocrreject_exam.ocrreject_exam(['odvkl1040', 'oe5ce1040' ,'odvkl1050'], plot=False, verbose=True)

For odvkl1040
Average across all extraction boxes: 31.8%
Average across all external regions: 0.8%
Average ratio between the two: 37.49

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
File ~/miniconda3/envs/stenv/lib/python3.11/site-packages/stistools/ocrreject_exam.py:165, in ocrreject_exam(obs_ids, data_dir, plot, plot_dir, interactive, verbose)
    161     if (instrument.strip() != 'STIS') or \
    162     (detector.strip() != 'CCD')    or \
    163     (obsmode.strip() != 'ACCUM')   or \
    164     (nextend < 6):
--> 165         raise ValueError
    166 except (KeyError, ValueError,):

ValueError: 

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
Cell In[5], line 1
----> 1 stistools.ocrreject_exam.ocrreject_exam(['odvkl1040', 'oe5ce1040' ,'odvkl1050'], plot=False, verbose=True)

File ~/miniconda3/envs/stenv/lib/python3.11/site-packages/stistools/ocrreject_exam.py:167, in ocrreject_exam(obs_ids, data_dir, plot, plot_dir, interactive, verbose)
    165         raise ValueError
    166 except (KeyError, ValueError,):
--> 167     raise ValueError(f"{flt_file}: Not a STIS/CCD/ACCUM file with ≥2 SCI extensions.")
    169 # If it is, proceed to check that the number of sci extensions matches the number of crsplits
    170 propid = flt_hdul[0].header['PROPOSID']

ValueError: ./oe5ce1040_flt.fits: Not a STIS/CCD/ACCUM file with ≥2 SCI extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants