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-1950: Bug fix for DQ flags #12

Merged
merged 4 commits into from
May 19, 2021

Conversation

kmacdonald-stsci
Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci commented May 18, 2021

The primary purpose of this PR is to fix the computation of the median of the first differences of a ramp. Because there is one less difference than there are groups a "shift" is needed to make sure the correct data quality flag is applied. This "shifting" was incorrectly done twice, when it should have only been done once.

Fixes JP-1950
Fixes spacetelescope/jwst#3879

…uppression. Also, made changes to fix the bug in JP-1950, where 'shifting' was happening twice.
@kmacdonald-stsci kmacdonald-stsci requested review from nden and dmggh May 18, 2021 18:10
dmggh
dmggh previously approved these changes May 18, 2021
@@ -1597,10 +1597,8 @@ def calc_slope(data_sect, gdq_sect, frame_time, opt_res, save_opt, rn_sect,
err_2d_array = data_sect[0, :, :] * frame_time

# Suppress, then re-enable, harmless arithmetic warnings
'''
Copy link
Collaborator

@jdavies-st jdavies-st May 18, 2021

Choose a reason for hiding this comment

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

You could use Numpy's errstate context manager here:

with np.errstate(divide="ignore", invalid="ignore"):
    err_2d_array[err_2d_array < 0] = 0

But that's just a more concise way of doing the same thing.

The current error/warning global config for numpy can be grabbed via np.geterr().

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question is why is e.g. divide by zero filtered out. Is there a chance it's going to be raised?
But since this is not related to the current bug being fixed I suggest if necessary it can be changed in another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the code changes not directly related to the bug in JP-1950.

@nden
Copy link
Collaborator

nden commented May 18, 2021

👍 merging after a change log is added.

@nden nden merged commit 0fc0bad into spacetelescope:main May 19, 2021
@kmacdonald-stsci kmacdonald-stsci deleted the jp_1950_new branch May 19, 2021 13:44
jdavies-st pushed a commit to spacetelescope/jwst that referenced this pull request May 20, 2021
* Updating test cases for the bug fix done in 
   spacetelescope/stcal#12

* bump install_requires and requirements-sdp.txt for stcal 0.2.1
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.

GroupDQ index bug in ramp_fit.py
5 participants