JP-1762: Add VA correction to WCS #5602

mcara · 2021-01-09T06:16:27Z

This PR adds support for velocity aberration correction in WCS. It needs thorough review.

It was decided, after a couple of iterations, that DVA correction will be applied as a separate transformation from the v2v3 frame to a new, VA-corrected, v2v3 frame that will be called 'v2v3vacorr' in WCS pipeline.

The code applies a constant scale DVA correction in the (almost-)tangent plane v2-v3 at Colin Cox suggestion. However, for when non-linear effects may become important, this PR also includes commented out full set of transformations from the spherical v2,v3 coordinates to a tangent plane and then applying constant scale transformation in that plane and then backward transformation to the (DVA-corrected) spherical coordinates v2,v3.

Resolves JP-1762 and #5434

codecov · 2021-01-09T06:49:37Z

Codecov Report

Merging #5602 (51767f2) into master (1135053) will increase coverage by 0.23%.
The diff coverage is 93.40%.

@@            Coverage Diff             @@
##           master    #5602      +/-   ##
==========================================
+ Coverage   75.40%   75.64%   +0.23%     
==========================================
  Files         406      406              
  Lines       36486    37214     +728     
==========================================
+ Hits        27513    28150     +637     
- Misses       8973     9064      +91

Flag	Coverage Δ		*Carryforward flag
nightly	`75.77% <94.91%> (ø)`		Carriedforward from 1135053
unit	`54.62% <88.23%> (+0.03%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
jwst/assign_wcs/fgs.py	`70.58% <80.00%> (-24.01%)`	⬇️
jwst/assign_wcs/niriss.py	`78.92% <85.71%> (+2.10%)`	⬆️
jwst/assign_wcs/miri.py	`85.26% <91.66%> (-4.37%)`	⬇️
jwst/assign_wcs/pointing.py	`94.35% <94.73%> (-0.78%)`	⬇️
jwst/assign_wcs/nirspec.py	`93.59% <96.42%> (-0.28%)`	⬇️
jwst/assign_wcs/nircam.py	`88.58% <100.00%> (+0.55%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1135053...51767f2. Read the comment docs.

jwst/assign_wcs/pointing.py

nden · 2021-01-11T00:53:09Z

jwst/assign_wcs/nirspec.py

@@ -142,6 +147,7 @@ def imaging(input_model, reference_files):
                            (sca, det2gwa),
                            (gwa, gwa2msa),
                            (msa_frame, None)]
+        # TODO: Add DVA correction


There's no need to add VA correction here since this pipeline does not go to sky but stops at the MSA.

nden · 2021-01-11T00:53:39Z

jwst/assign_wcs/nirspec.py

@@ -231,12 +237,13 @@ def ifu(input_model, reference_files, slit_y_range=[-.55, .55]):
                    (slit_frame, slit2slicer),
                    ('slicer', slicer2msa),
                    (msa_frame, None)]
+        # TODO: Add DVA correction


No need for VA correction here, ti stops at the MSA.

nden · 2021-01-11T00:54:15Z

jwst/assign_wcs/nirspec.py

@@ -345,14 +352,15 @@ def slitlets_wcs(input_model, reference_files, open_slits_id):
                        (gwa, gwa2slit),
                        (slit_frame, slit2msa),
                        (msa_frame, None)]
+        # TODO: Add DVA correction


and here, no va correction

mcara · 2021-01-12T20:27:10Z

Thanks @nden! I removed TODO comments, unused imports and fixed a bug due to which v2_ref and v3_ref were not converted to degrees.

jdavies-st

This looks good!

Just a couple minor comments below. Not sure why we're seeing small changes in the computed coordinates, mostly in S_REGION.

https://plwishmaster.stsci.edu:8081/job/RT/job/jdavies-dev/220/testReport/

jdavies-st · 2021-02-17T20:50:24Z

jwst/assign_wcs/miri.py

+        name='v2v3vacorr_spatial',
+        axes_order=(0, 1),
+        unit=(u.arcsec, u.arcsec),
+        axes_names=('v2', 'v3')


Is there a reason to have axes_names here but not for the MIRI imaging mode (or the other instrument imaging modes)?

I do not know... I just copied the frame that was already used in the assign_wcs for a given instrument. I simply appended vacorr to the frame name. No other changes. I also noticed sometimes frames are called v2 and sometimes V3 - see #5746 point #4. Maybe this should be noted there?

I see. Maybe for a future "consistency and cleanup" PR then. Agree to keep it as for this PR.

jdavies-st · 2021-02-17T20:53:12Z

jwst/assign_wcs/nircam.py

@@ -74,6 +74,7 @@ def imaging(input_model, reference_files):
    """
    detector = cf.Frame2D(name='detector', axes_order=(0, 1), unit=(u.pix, u.pix))
    v2v3 = cf.Frame2D(name='v2v3', axes_order=(0, 1), unit=(u.arcsec, u.arcsec))
+    v2v3vacorr = cf.Frame2D(name='v2v3vacorr', axes_order=(0, 1), unit=(u.arcsec, u.arcsec))


Again, no axes_names here. Maybe it would be good to have consistency between these imaging modes and the spectral modes?

jdavies-st · 2021-02-17T20:56:58Z

jwst/assign_wcs/pointing.py

+            va_scale = float(datamodel.meta.velocity_aberration.scale_factor)
+            v2_ref = float(datamodel.meta.wcsinfo.v2_ref)
+            v3_ref = float(datamodel.meta.wcsinfo.v3_ref)


Casting these to float is probably not needed, as they are already floats by definition from the schemas. Or they are not populated and return None, which of course will return a TypeError when cast to float. AttributeError should never be returned.

You could just put the equation in the try block and if any of the items are None, you're going to get a TypeError.

I wonder the casting of values could be causing the slightly differences in the regtests for S_REGION? One is type <class 'numpy.float32'>, the other <class 'float'>.

You could just put the equation in the try block and if any of the items are None, you're going to get a TypeError.

Isn't what I am already doing? I did not know that meta.velocity_aberration.scale_factor and wcsinfo.v2_ref and meta.wcsinfo.v3_ref are always guaranteed to exist. If that is the case, I'll be happy to remove attribute error from being caught.

Yeah, they are always guaranteed to exist because they're in the schema and in meta.

jdavies-st · 2021-02-17T21:13:39Z

jwst/assign_wcs/pointing.py

+    #     rot.inverse | c2s | unit_conv.inverse
+    # )
+
+    va_corr.name = 'VA_Correction'


The name should be descriptive, and bits are free. "Spacecraft Velocity Aberration Correction" would be useful.

@nden What do you think? I personally think that VA_Correction should be OK but if you think a longer string is more desirable - it is easy to change this.

Models can be referenced by name so I'd say shorter is better and not having spaces in the name is preferable.

jdavies-st · 2021-02-17T21:14:50Z

jwst/assign_wcs/tests/test_pointing.py

+def create_imaging_datamodel(v2_ref, v3_ref, va_scale):
+    hdul = create_hdul()
+    datamodel = ImageModel(hdul)
+    datamodel.meta.velocity_aberration.scale_factor = va_scale
+    datamodel.meta.wcsinfo.v2_ref = v2_ref
+    datamodel.meta.wcsinfo.v3_ref = v3_ref
+    return datamodel


This could be a Pytest fixture, and it would be much clearer that the tests depend on it.

mcara · 2021-02-18T03:12:59Z

Just a couple minor comments below. Not sure why we're seeing small changes in the computed coordinates, mostly in S_REGION.

https://plwishmaster.stsci.edu:8081/job/RT/job/jdavies-dev/220/testReport/

I investigated this and it looks that VA_SCALE was poulated with a value != 1 and hence now that VA correction is accounted for in the WCS, changes in produced world coordinates are expected. I think what we see here is reasonable.

For example, for test_miri_lrs_slit_spec2_run_pipeline0/jw00623032001_03102_00001_mirimage_cal.fits I get:

In [7]: d.meta.velocity_aberration.scale_factor
Out[7]: 1.000000146732572

jdavies-st

Thanks Mihai. Yeah, it looks like some of our test data, probably from OTB or SDP processing, already has VA_SCALE set, so nice to see it exercised in your code.

nden

LGTM other than the code style issue

nden · 2021-02-18T20:55:28Z

Needs a change log.

mcara added assign_wcs wcs velocity_aberration labels Jan 9, 2021

mcara requested a review from nden January 9, 2021 06:16

mcara self-assigned this Jan 9, 2021

nden reviewed Jan 11, 2021

View reviewed changes

jdavies-st changed the title ~~Add VA correction to WCS~~ JP-1762: Add VA correction to WCS Jan 11, 2021

mcara force-pushed the add-va-corr branch from 0733066 to c919002 Compare January 12, 2021 20:20

stscijgbot-jp mentioned this pull request Jan 19, 2021

Apply VA_SCALE to JWST WCS objects #5434

Closed

mcara mentioned this pull request Jan 23, 2021

Computation of velocity aberrated RA, DEC and scale #5658

Closed

mcara force-pushed the add-va-corr branch from ab5e800 to 2c62883 Compare January 29, 2021 06:45

mcara force-pushed the add-va-corr branch from 22de93c to fc15b30 Compare February 12, 2021 20:51

mcara requested a review from nden February 12, 2021 22:28

mcara mentioned this pull request Feb 16, 2021

Add support for JWST VA corrected V2V3 frames spacetelescope/tweakwcs#130

Merged

mcara force-pushed the add-va-corr branch 2 times, most recently from a076f38 to 4107925 Compare February 17, 2021 20:23

mcara requested a review from jdavies-st February 17, 2021 20:24

jdavies-st reviewed Feb 17, 2021

View reviewed changes

jdavies-st approved these changes Feb 18, 2021

View reviewed changes

nden approved these changes Feb 18, 2021

View reviewed changes

mcara added 6 commits February 18, 2021 16:39

Add VA correction to WCS

26eb74f

Add fast algorithm for VA correction based on Colin Cox approximation

02518e8

Comment out long computations using Euler rot and projections

914252c

Move VA correction to its own WCS pipeline step/frame

4f6e398

Update tweakreg version requirement

32587bb

Major refactoring of this PR

3e975ce

Add log entry and fix flake8

51767f2

mcara force-pushed the add-va-corr branch from 33e60d9 to 51767f2 Compare February 18, 2021 21:41

mcara merged commit 2ff9864 into spacetelescope:master Feb 18, 2021

mcara mentioned this pull request Feb 18, 2021

JP-1931: Add/Rename V2-V3 frame axes names #5765

Merged

stscijgbot-jp mentioned this pull request Feb 19, 2021

Some V2V3 frames are missing axes names and have wrong units #5770

Closed

mcara deleted the add-va-corr branch May 13, 2021 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-1762: Add VA correction to WCS #5602

JP-1762: Add VA correction to WCS #5602

mcara commented Jan 9, 2021 •

edited by hbushouse

Loading

codecov bot commented Jan 9, 2021 •

edited

Loading

nden Jan 11, 2021

nden Jan 11, 2021

nden Jan 11, 2021

mcara commented Jan 12, 2021

jdavies-st left a comment

jdavies-st Feb 17, 2021 •

edited

Loading

mcara Feb 18, 2021

jdavies-st Feb 18, 2021

jdavies-st Feb 17, 2021

jdavies-st Feb 17, 2021 •

edited

Loading

mcara Feb 18, 2021

jdavies-st Feb 18, 2021

jdavies-st Feb 17, 2021

mcara Feb 18, 2021

nden Feb 18, 2021

jdavies-st Feb 17, 2021

mcara commented Feb 18, 2021 •

edited

Loading

jdavies-st left a comment

nden left a comment

nden commented Feb 18, 2021

JP-1762: Add VA correction to WCS #5602

JP-1762: Add VA correction to WCS #5602

Conversation

mcara commented Jan 9, 2021 • edited by hbushouse Loading

codecov bot commented Jan 9, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara commented Jan 12, 2021

jdavies-st left a comment

Choose a reason for hiding this comment

jdavies-st Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdavies-st Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara commented Feb 18, 2021 • edited Loading

jdavies-st left a comment

Choose a reason for hiding this comment

nden left a comment

Choose a reason for hiding this comment

nden commented Feb 18, 2021

mcara commented Jan 9, 2021 •

edited by hbushouse

Loading

codecov bot commented Jan 9, 2021 •

edited

Loading

jdavies-st Feb 17, 2021 •

edited

Loading

jdavies-st Feb 17, 2021 •

edited

Loading

mcara commented Feb 18, 2021 •

edited

Loading