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

JP-3737: Handle micrometeorite flashes #308

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

drlaw1558
Copy link
Contributor

This PR addresses JP-3737 by adding an additional check in the jump step to look for micrometeorite flashes, which manifest as bursts of jumps across a large fraction of the detector.

Kept in draft form for now pending potential future modifications.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 18.18182% with 18 lines in your changes missing coverage. Please review.

Project coverage is 86.24%. Comparing base (60bd3b8) to head (66f0bb6).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/jump/jump.py 18.18% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   86.21%   86.24%   +0.03%     
==========================================
  Files          47       49       +2     
  Lines        8812     8876      +64     
==========================================
+ Hits         7597     7655      +58     
- Misses       1215     1221       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some code called in two separate places due to a conditional, but can be moved outside the conditionals and called once, since they are called in both the if and else portion of the conditional.

There is also some formatting and style issues.

@@ -338,6 +344,12 @@ def detect_jumps(
)
log.info("Total showers= %i", num_showers)
number_extended_events = num_showers

# This is where we look for micrometeorite flashes if the triggering
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is done in both conditionals and done last, it should be moved out side the conditionals completely, instead of invoked twice.

The expand_large_events and find_showers can be moved outside the conditionals for the same reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


# This is where we look for micrometeorite flashes if the triggering
# threshold is less than the entire array
if (mmflashfrac < 1):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is done in both conditionals and done last, it should be moved out side the conditionals completely, instead of invoked twice.

The expand_large_events and find_showers can be moved outside the conditionals for the same reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# of groups. Do this by looking for cases where greater than mmflash_pct percent
# of the array is flagged as 'JUMP_DET' and flag the entire array.
def find_micrometeorite_flashes(gdq, jump_flag, mmflashfrac):
npix = gdq.shape[2]*gdq.shape[3] # Number of pixels in array
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add spaces before and after *.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

def find_micrometeorite_flashes(gdq, jump_flag, mmflashfrac):
npix = gdq.shape[2]*gdq.shape[3] # Number of pixels in array
# Loop over integrations
for ii in range(0,gdq.shape[0]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# Loop over integrations
for ii in range(0,gdq.shape[0]):
# Loop over groups
for jj in range(0,gdq.shape[1]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# Find micrometeorite flashes that light up the entire array in a small number
# of groups. Do this by looking for cases where greater than mmflash_pct percent
# of the array is flagged as 'JUMP_DET' and flag the entire array.
def find_micrometeorite_flashes(gdq, jump_flag, mmflashfrac):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a docstring.

Also, start the function with nints, ngroups, nrows, ncols = gdq.shape, then use these more descriptive variable names, rather than shape[k].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point- done

for ii in range(0,gdq.shape[0]):
# Loop over groups
for jj in range(0,gdq.shape[1]):
indx = np.where(gdq[ii,jj,:,:] & jump_flag != 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add spaces after all ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

indx = np.where(gdq[ii,jj,:,:] & jump_flag != 0)
fraction = float(len(indx[0])) / npix
if (fraction > mmflashfrac):
gdq[ii,jj,:,:] = gdq[ii,jj,:,:] | jump_flag
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add spaces after all ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fraction = float(len(indx[0])) / npix
if (fraction > mmflashfrac):
gdq[ii,jj,:,:] = gdq[ii,jj,:,:] | jump_flag
log.info("Detected a micrometeorite flash in integ = %i, group= %i", ii, jj)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use an f-string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the convention I certainly can- done.

@drlaw1558
Copy link
Contributor Author

Things that remain to be determined:

  • Check that this function successfully detects more example flashes from the FGS list
  • Determine whether it needs to be restricted in any way, e.g. to only FULL array or non-TSO data
  • Determine whether to set mmflashfrac=1.0 by default (i.e., step disabled unless explicitly enabled by param ref file), or to set mmflashfrac=0.1 (for example) to explicitly enable by default unless disabled by param ref file.

Work ongoing to determine these.

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

Successfully merging this pull request may close these issues.

2 participants