-
Notifications
You must be signed in to change notification settings - Fork 156
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-2090: Update set_velocity_aberration to use DataModel #8285
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8285 +/- ##
==========================================
+ Coverage 75.15% 75.37% +0.21%
==========================================
Files 470 474 +4
Lines 38604 38921 +317
==========================================
+ Hits 29014 29337 +323
+ Misses 9590 9584 -6
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Did you do a regression test run? I'm not sure we have any tests that cover this script, but it can't hurt. |
I did not - the test coverage is inside lib/tests/test_velocity_aberration.py. I will do a regression test run anyway, though |
started this Jenkins run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These code updates look fine. But before we approve and merge, I'm doing some research into JP-896, which suggests we might need/want to add another fix to this script, in order to have it use velocity information in the proper reference frame. That ticket suggests that these calculations should be using velocities that are in the heliocentric frame, but the keyword values we're using are actually in the Earth-centered frame. I'm trying to track down exactly what's going on with all of that.
Co-authored-by: Howard Bushouse <bushouse@stsci.edu>
Regression test run results are clean and JP-896 has been verified to no longer be an issue (fixed long ago in DMS build 7.8), so no further updates are needed to fix any old issues in this PR. |
Resolves JP-2090
Closes #6053
This PR addresses updating the set_velocity_aberration script to use DataModels instead of fits for opening and manipulating data.
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR