RCAL-853: Allow ability to introduct velocity aberration scale factor

[stscieisenhamer]

WIP RCAL-853

Main PR: PR#1354

This PR adds the velocity aberration transformation to the WCS to determine the FOV of the resulting L1 simulations. In addition, the meta velocity_aberration.scale_factor is populated so that the ELP pipeline step assign_wcs correctly accounts for the applied aberration.

[codecov]

Codecov Report

Attention: Patch coverage is 58.20896% with 28 lines in your changes missing coverage. Please review.

Project coverage is 90.02%. Comparing base (81028a8) to head (244858c).
Report is 48 commits behind head on main.

Files with missing lines	Patch %	Lines
romanisim/velocity_aberration.py	33.33%	14 Missing ⚠️
romanisim/wcs.py	57.89%	8 Missing ⚠️
romanisim/util.py	58.33%	5 Missing ⚠️
romanisim/ris_make_utils.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   89.24%   90.02%   +0.78%     
==========================================
  Files          17       17              
  Lines        2073     2165      +92     
==========================================
+ Hits         1850     1949      +99     
+ Misses        223      216       -7     

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

[schlafly]

Jonathan and I talked about this on Friday, but recording some of my thoughts here.

I think we need to add some bits around here https://github.com/spacetelescope/romanisim/blob/main/romanisim/util.py#L302-L309 . The issue is that this correction produces the right WCS (i.e., the one that would be observed if the aberrated WFI_CEN were at the reference ra/dec. But there is a bit of code to populate the corresponding wcsinfo centered at the detector, and I don't think we'll do the right thing there at present. In particular that code assumes that wcs(center) = radec_ref, but I think for the romancal WCS that's not quite right when aberration is non-zero. Instead the point whose v2v3a = center goes to radec_ref. Probably the easiest way to get that point is to extract the part of the WCS from v2v3a to sky and evaluate radec_ref = v2v3a_to_sky(v2v3). Likewise roll_ref needs to work from this frame and so radecn need to be evaluated there.

I think that's everything? But the proof at the end of the day is a check that the romanisim WCS exactly matches the romancal WCS for some non-trivial scale factor.

We also should populate the ephemerides so that the scale factors get computed. I suggest doing this by assuming that the telescope is exactly at L2 and neglecting more details of the orbit.

[schlafly]

This all looks good to me. Can I ask you to add one of your consistency check plots to this PR, and one or two paragraphs to the docs here?

https://romanisim.readthedocs.io/en/latest/romanisim/wcs.html

Please mention the typical scale of the correction (i.e., a value of va_scale). I'd also mention in a sentence that we make a wcsinfo for each individual detector but internally our WCS in centered on wfi_cen or the boresight, and differing treatment of the spherical to cartesian transformations makes a ~10^-6 arcsec effect.

[stscieisenhamer]

Docs updated. Also changed the command-line default to use a scale of 1.0.

[schlafly]

Looks good to me. I left a couple of minor comments inline but feel free to merge once those are addressed.

[schlafly]

Just since I see it... the policy in other places in romanisim has been to do something like

from romanisim import log
log.info(...)

This has never worked great, which I have blamed on weird interactions with stpipe. Is this doing something good on top of that?

[stscieisenhamer]

The form in the code is generally used for library/utility modules. This basically leaves the logging up to whatever higher level code is using the module. For edification, this is where it comes from.

However, if the rest of romanisim uses the another standard, as suggested, there is no real issue in changing it. I'll change it so it is consistent with the rest of the package.

About the weirdness with stpipe: The is unfortunately an stpipe problem, mainly because stpipe violates what is suggested in the aforementioned link. Without some hoop-jumping, until stpipe gets it stuff together, there is not much to do about the weirdness.

[schlafly]

Right. But I think that that code would belong around here?
https://github.com/spacetelescope/romanisim/blob/main/romanisim/__init__.py#L15-L20
Then other modules wouldn't need it? I might be missing something about what doing it in romanisim.velocity_aberration does relative to doing things module-wide.

And it looks like I don't set a default handler there, and maybe I should? Then all of the other modules would likewise benefit.

I will certainly admit that to me it seems python backed itself pretty far into a corner on logging, which is hard to get right!

[schlafly]

Let's make the default -1, or no one will ever use this.

[stscieisenhamer]

Do not understand the build issues, but those will be unrelated to this PR. Merging away.