Improve code for computing VA scale and offsets #5666

mcara · 2021-01-26T06:36:07Z

This PR improves code in lib/set_velocity_aberration.py based on suggestions from Colin Cox:

Simplify formula for computing constant scale (neglect higher-order terms);
Improve formula for computing RA, DEC offsets based on example code supplied by Colin Cox;
Overall refactoring.

Closes: #5658
Related to JP-1762

hbushouse · 2021-01-26T14:22:57Z

Need to update unit tests in test_velocity_aberration.py, which are causing CI to fail.

hbushouse

Code refactoring looks good to me. Just need to fix the the unit tests that are failing.

mcara · 2021-01-26T21:00:13Z

@hbushouse @ColinRCox @antonkoekemoer @nden Could you please take a look at #5658 and comment on the issues either here or there or ping someone who is knowledgeable in how this code and its computed values are intended to be used.

Thanks!

codecov · 2021-01-26T21:17:28Z

Codecov Report

Merging #5666 (3106945) into master (e549311) will decrease coverage by 0.00%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##           master    #5666      +/-   ##
==========================================
- Coverage   74.46%   74.45%   -0.01%     
==========================================
  Files         416      416              
  Lines       38110    38127      +17     
  Branches     5000     5000              
==========================================
+ Hits        28378    28387       +9     
- Misses       9732     9740       +8

Flag	Coverage Δ		*Carryforward flag
nightly	`76.00% <100.00%> (ø)`		Carriedforward from e549311
unit	`54.16% <81.81%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
jwst/lib/set_velocity_aberration.py	`88.40% <84.00%> (-11.60%)`	⬇️

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 e549311...3106945. Read the comment docs.

mcara · 2021-01-27T03:12:47Z

@hbushouse I have made several significant changes to code and schemas. Please re-review this PR. For schema review, maybe @jdavies-st could take a look (don't I need to update a schema version somewhere?)

Here is the outline of changes:

I have combined computation of scale with the computation of apparent RA and DEC;
Instead of "offsets" - the differences between RA (DEC) in the moving telescope and barycentric frames, now the values in the moving frame are reported (apparent RA and DEC);
FITS keywords have been changed to VA_RA and VA_DEC (I wanted to have VA_REF_DEC but it does not fit into 8 characters; the reason for REF is that script computes VA angles of meta.wcsinfo.ra_ref and meta.wcsinfo.dec_ref; the reason I renamed DVA to VA is because D comes from differential which, I believe, does not apply to these quantities).
VA_RA and VA_DEC are returned in degrees and not radians;
In jwst/datamodels/schemas/core.schema.yaml I have renamed ra(dec)_offset to va_ra(dec)_ref;
I have somewhat modified Colin Cox' code to return RA in the range [0, 360) instead of [-180, 180) (by re-using gwcs.geometry.CartesianToSpherical model).
Updated unit tests. Previous tests were using RA_ref=0 and DEC_ref=0 which made test for the case of zero velocity to not be able to distinguish between offsets and apparent angle values.

hbushouse · 2021-01-27T13:54:57Z

CHANGES.rst

+- Update code in ``set_velocity_aberration.py`` functions based on Colin Cox
+  suggestions (simplify DVA scale computation and improve offset
+  computation). [#5666]
+


Should also now add an entry to the datamodels section above, to indicate the change in the keyword names DVA_RA, DVA_DEC to VA_RA, VA_DEC.

yes, I realized that too late. I'll add more to the description.

hbushouse · 2021-01-27T13:59:08Z

jwst/datamodels/schemas/core.schema.yaml

@@ -1293,15 +1293,15 @@ properties:
        title: Velocity aberration correction information
        type: object
        properties:
-          ra_offset:
-            title: "[rad] Velocity aberration correction RA offset"
+          va_ra_ref:


The "REF" suffix in other keywords like RA_REF, DEC_REF indicates that these are the coordinates at the reference point of the aperture/image. If VA_RA, VA_DEC are simply the VA-correction target coords, then the "ref" suffix is probably not appropriate, because the target is not necessarily located at the reference point (e.g. dithers, mosaics, and other intentional offsets move the target off the reference point). So unless these RA, Dec values are at the reference point, we should probably drop the "ref" suffix from their datamodel names and just leave them as "va_ra" and "va_dec".

So unless there RA, Dec values are at the reference point ...

It seems that they are. Take a look at the original code:

jwst/jwst/lib/set_velocity_aberration.py

Lines 143 to 150 in 1f4db5c

ra_ref = float(sheader['RA_REF'])

dec_ref = float(sheader['DEC_REF'])

# compute the velocity aberration information

scale_factor = aberration_scale(jwst_dx, jwst_dy, jwst_dz,

ra_ref, dec_ref)

ra_off, dec_off = aberration_offset(jwst_dx, jwst_dy, jwst_dz,

ra_ref, dec_ref)

In the script in the functions that compute VA effects, input arguments are called targ_ra and targ_dec and I find this confusing. In the function that updates FITS header, targ_ra=ra_ref, ... I think it is important to preserve the actual meaning of the keywords.

Ah ha, so it appears that these coords are in fact the RA, Dec of the reference point, which is not necessarily the same as the coords of the target (if the target isn't at the reference point). Yes, it is unfortunate and confusing when the variable names used are "targ_ra" and "targ_dec", which makes you think it's the same as the TARG_RA, TARG_DEC keywords, but in fact they correspond to keywords RA_REF, DEC_REF. So in that case I withdraw my previous comment and think it's OK to leave the "ref" suffix on the meta attribute names. Too bad the stupid FITS standard won't allow us the room to add "REF" to the keyword names too.

I was writing #5666 (comment) in parallel with you. I was suggesting to add at least an R in front of RA->RRA and DEC->RDEC just to alert readers and make it obvious that something is different about this RA and DEC.

hbushouse · 2021-01-27T14:01:01Z

jwst/datamodels/schemas/core.schema.yaml

-          dec_offset:
-            title: "[rad] Velocity aberration correction Dec offset"
+          va_dec_ref:
+            title: "[dec] Velocity aberration apparent reference Dec"


typo in "dec" - should be "deg"

hbushouse · 2021-01-27T14:04:07Z

jwst/lib/set_velocity_aberration.py

-    # angle between the velocity vector and the direction toward the target.
-    sin_theta = math.sqrt(1. - cos_theta**2)
+    apparent_ra: float
+        Aparent star position in the moving telescope frame.


typo: "Aparent" should be "Apparent"

hbushouse · 2021-01-27T14:04:40Z

jwst/lib/set_velocity_aberration.py

-    tan_theta_p = sin_theta / (gamma * (cos_theta + beta))
-    theta_p = math.atan(tan_theta_p)
+    apparent_dec: float
+        Aparent star position in the moving telescope frame.


same typo here "Apparent"

hbushouse · 2021-01-27T14:07:02Z

jwst/lib/set_velocity_aberration.py

+    beta = np.array([velocity_x, velocity_y, velocity_z]) / SPEED_OF_LIGHT
+    beta2 = np.dot(beta, beta)
+    if beta2 == 0.0:
+        return 1.0, targ_ra, targ_dec


The old version of the code issued a warning when the velocity is zero. That would be good to maintain here, just for traceability in the processing log.

The old warning logger.warning('Speed is zero. Forcing scale to 1.0') is interesting. When speed is 0 then mathematically scale is 1. There is nothing exceptional here. Saying the code is "forcing" scale to be 1 is kind of a forceful statement :) I think from the caller's perspective it should not matter how the code handles special cases. In fact, this if can be safely removed from the code if we are talking only about scale. It is here because of the aberrated RA and Dec - that formula should be taking a limit in mathematical sense when v->0. In addition, it is really an extreme check to make code work if when JWST is "launched" in a garage. We do not realy expect a flying JWST with speed 0.

However, if you are thinking of detecting case when software incorrectly passes 0 speed instead of expected v != 0 - then I hope someone will be reading those log files. Otherwise I would suggest using an assert or raising some other exception.

Let me know what do you prefer.

ColinRCox · 2021-01-27T16:03:16Z

A zero speed will not interfere with the scale calculation but the RA and Dec calculation will have a divide by zero problem. By the way beta2 is the square of the velocity so I would prefer to name it something like beta_squared to make it clear.

mcara · 2021-01-27T18:29:11Z

@hbushouse I addressed (locally) most comments. I have a question about the use of REF(ref). Are you OK with va_ref_ra (or maybe make it va_ra_ref to make it consistent with wcsinfo.ra_ref)? Also, what about changing FITS keywords from VA_RA and VA_DEC to VA_RFDEC (the most I can fit in 8 characters) or VA_RDEC?

The log message is back but modified:

logger.warning('Observatory speed is zero. Setting VA scale to 1.0')

I hope we will never see this message.

ColinRCox · 2021-02-01T15:48:54Z

The formula for computing the scale is exact. It is fairly simple but no higher terms have been omitted.

mcara added the velocity_aberration label Jan 26, 2021

mcara requested review from nden and hbushouse January 26, 2021 06:36

mcara self-assigned this Jan 26, 2021

mcara force-pushed the update_va_corr_computations branch from a5b8ce7 to 1008077 Compare January 26, 2021 06:38

hbushouse reviewed Jan 26, 2021

View reviewed changes

hbushouse added this to the Build 7.7.x milestone Jan 26, 2021

mcara force-pushed the update_va_corr_computations branch from c74ceda to fddc5e7 Compare January 26, 2021 20:44

mcara force-pushed the update_va_corr_computations branch from e99dc86 to 1f80777 Compare January 27, 2021 02:47

mcara requested a review from hbushouse January 27, 2021 02:48

mcara force-pushed the update_va_corr_computations branch 2 times, most recently from d37d03e to d10b29b Compare January 27, 2021 03:54

hbushouse reviewed Jan 27, 2021

View reviewed changes

mcara force-pushed the update_va_corr_computations branch from ae56d01 to 6a641fb Compare January 27, 2021 19:56

mcara added 3 commits January 27, 2021 15:40

Improve code for computing VA scale and offsets

768d1d3

Change offsets to apparent position. Combine functions

4e82712

Address review comments

3106945

mcara force-pushed the update_va_corr_computations branch from 312d2f4 to 3106945 Compare January 27, 2021 20:41

hbushouse approved these changes Jan 27, 2021

View reviewed changes

hbushouse merged commit 94723d0 into spacetelescope:master Jan 28, 2021

stscijgbot-jp mentioned this pull request Feb 5, 2021

Apply VA_SCALE to JWST WCS objects #5434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code for computing VA scale and offsets #5666

Improve code for computing VA scale and offsets #5666

mcara commented Jan 26, 2021 •

edited by hbushouse

Loading

hbushouse commented Jan 26, 2021

hbushouse left a comment

mcara commented Jan 26, 2021

codecov bot commented Jan 26, 2021 •

edited

Loading

mcara commented Jan 27, 2021 •

edited

Loading

hbushouse Jan 27, 2021

mcara Jan 27, 2021

hbushouse Jan 27, 2021

mcara Jan 27, 2021

hbushouse Jan 27, 2021

mcara Jan 27, 2021

hbushouse Jan 27, 2021

hbushouse Jan 27, 2021

hbushouse Jan 27, 2021

hbushouse Jan 27, 2021

mcara Jan 27, 2021 •

edited

Loading

mcara Jan 27, 2021

ColinRCox commented Jan 27, 2021

mcara commented Jan 27, 2021 •

edited

Loading

ColinRCox commented Feb 1, 2021

	ra_ref = float(sheader['RA_REF'])
	dec_ref = float(sheader['DEC_REF'])

	# compute the velocity aberration information
	scale_factor = aberration_scale(jwst_dx, jwst_dy, jwst_dz,
	ra_ref, dec_ref)
	ra_off, dec_off = aberration_offset(jwst_dx, jwst_dy, jwst_dz,
	ra_ref, dec_ref)

Improve code for computing VA scale and offsets #5666

Improve code for computing VA scale and offsets #5666

Conversation

mcara commented Jan 26, 2021 • edited by hbushouse Loading

hbushouse commented Jan 26, 2021

hbushouse left a comment

Choose a reason for hiding this comment

mcara commented Jan 26, 2021

codecov bot commented Jan 26, 2021 • edited Loading

Codecov Report

mcara commented Jan 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinRCox commented Jan 27, 2021

mcara commented Jan 27, 2021 • edited Loading

ColinRCox commented Feb 1, 2021

mcara commented Jan 26, 2021 •

edited by hbushouse

Loading

codecov bot commented Jan 26, 2021 •

edited

Loading

mcara commented Jan 27, 2021 •

edited

Loading

mcara Jan 27, 2021 •

edited

Loading

mcara commented Jan 27, 2021 •

edited

Loading