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

RCAL-853: Apply velocity aberration correction to the WFI WCS #1354

Merged

Conversation

stscieisenhamer
Copy link
Collaborator

@stscieisenhamer stscieisenhamer commented Aug 8, 2024

Resolves RCAL-853

Closes #1192

This PR applies the velocity aberration correction to the WFI WCS.

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@stscieisenhamer
Copy link
Collaborator Author

@schlafly Current PR includes basically a duplicate of the JWST correction, that of applying the single scale as given by meta.velocity_aberration.scale_factor.

However, the questions in the issues relate more to whether scale_factor is sufficiently close to what ra_offset/dec_offset are, such that they are indistinguishable, or, more importantly, no systematic effect is introduced using the single value.

Nominally, all these values are being calculated in RSDP. Are we tasked to provide those calculations, and hence also deciding whether the single scale_factor or the individual offsets are to be used in the correction? Towards this, I am getting some sample data and seeing about making some empirical estimates.

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 36.00000% with 16 lines in your changes missing coverage. Please review.

Project coverage is 76.55%. Comparing base (6a03543) to head (ad5df32).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
romancal/assign_wcs/pointing.py 30.43% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1354      +/-   ##
==========================================
- Coverage   76.69%   76.55%   -0.14%     
==========================================
  Files         120      120              
  Lines        7830     7854      +24     
==========================================
+ Hits         6005     6013       +8     
- Misses       1825     1841      +16     

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

@schlafly
Copy link
Collaborator

schlafly commented Aug 8, 2024

Hi Jonathan,

We will need to compute scale_factor. I think that's a simple function of the velocity of the spacecraft---some beta or gamma factor? I found the computation of that factor in the Webb code, and put the link in the issue.

It depends on the speed of the spacecraft, which is more annoying to estimate. I think the easiest thing to do would be to pretend that the spacecraft were exactly at L2, so that the spacecraft's velocity is exactly the same as the Earth's velocity times (l2_distance_from_sun / earth_distance_from_sun), where I guess I'm ignoring effects from the moon but that seems very small.

I had not appreciated until looking at your code that that the ra_ref and dec_ref correspond to the aberrated v2/v3 rather than the nominal v2/v3. Those ra/dec_ref come from romanisim at present and so I think it is straightforward to get that right. But I'm rather attached to the notion than ra_ref, dec_ref corresponds to v2_ref, v3_ref, which I claim this will break. Is that how things work for Webb?

@stscieisenhamer
Copy link
Collaborator Author

The previous comment generated much offline discussion. The summary and path forward is below; let me know if I got it wrong.

Concerning where the computation of va_scale occurs: For JWST, this is done in SDP using jwst cal code while the Level 1 file is built. For Roman, this can be done as JWST, or in the set_telescope_pointing equivalent.

For JWST, as described, the RA_REF/DEC_REF are the "semi-aberrated" versions. These values are determined in set_telescope_pointing and are calculated using the SIAF transformation from V1. V1 has been corrected. However, the SIAF transformation does not take into account the differential aberration, which is why the jwst wcs models add the shift component. These values are never updated in the headers. For Roman, these values can be updated in the assign_wcs step, or the set_telescope_pointing equivalent can take into account the DVA in the first place.

Just recently learned (27Aug24) about the plans on back-porting the Level3/GAIA corrected solution to the Level 2 files. Details of that does not matter. Note that this process will invalidate all the wcsinfo keywords.

For the moment, I believe the plan is to leave the assign_wcs code as is. I can add the updating of the wcsinfo to ensure that RA_REF/DEC_REF represents what V2_REF/V3_REF is pointing at. Note that this updating will use the WCS object which will also include distortion.

@schlafly
Copy link
Collaborator

schlafly commented Aug 27, 2024

It's fine to compute va_scale in romanisim (when the L1s are produced).

Let's follow Webb for ra/dec ref. i.e., radec_ref only match v1v2_ref exactly for the boresight; the others don't match up exactly due to differential aberration but they're within a pixel or so. This is tracked in the WCS but not in the wcsinfo. I'll add an issue to rad (spacetelescope/rad#437) bringing up whether we want to add v2a/v3a_ref which would allow these to match up better, though still not perfectly after tweakreg has been run since we don't update the wcsinfo with tweakreg information.

Re backporting the L2 tweakreg WCSes to the L1 files, we'll end up doing something like providing a pile of L2 WCS files separately from the rest of the L2s (which would include tweakreg, aberration tweaks) or just the tweakreg info plus code that turns the wcsinfo + tweakreg info into a new WCS (basically, repeating assign_wcs, aberration, ...). I don't think that invalidates the wcsinfo keywords any worse than they are already invalid in the L2s.

Let's not update ra_ref/dec_ref yet and instead just continue following Webb.

@stscieisenhamer stscieisenhamer force-pushed the rcal-853-velocity branch 2 times, most recently from b55bdda to 4ec8d20 Compare November 15, 2024 19:44
@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file labels Nov 15, 2024
@stscieisenhamer stscieisenhamer marked this pull request as ready for review November 15, 2024 21:49
@stscieisenhamer stscieisenhamer requested a review from a team as a code owner November 15, 2024 21:49
@stscieisenhamer stscieisenhamer force-pushed the rcal-853-velocity branch 2 times, most recently from b813666 to 4f8150c Compare November 26, 2024 17:16
@stscieisenhamer
Copy link
Collaborator Author

Regression test (I believe)

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Regtests are passing; this can go ahead, thanks. It won't start actually doing something until we start making files with velocities in romanisim; that will start happening when we have merged spacetelescope/romanisim#162 and make new files.

from gwcs.geometry import CartesianToSpherical, SphericalToCartesian

log = logging.getLogger(__name__)
log.setLevel(logging.DEBUG)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't do this elsewhere AFAIK; was this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Missed this cut and paste error. Removing.

romancal/assign_wcs/pointing.py Show resolved Hide resolved
@stscieisenhamer stscieisenhamer merged commit 20b1fdf into spacetelescope:main Dec 3, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assign_wcs dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement velocity aberration in AssignWCS
2 participants