spacetelescope / pysiaf

Handling of Science Instrument Aperture Files (SIAF) for space telescopes
https://pysiaf.readthedocs.io
BSD 3-Clause "New" or "Revised" License
13 stars 28 forks source link

Update or remove test_jwst_aperture_transforms #186

Open mfixstsci opened 3 years ago

mfixstsci commented 3 years ago

https://github.com/spacetelescope/pysiaf/blob/master/pysiaf/tests/test_aperture.py#L114

From @drlaw1558

Lines 114/115 test the roundtrip transformations at xsci/ysci = -10, 0, +10 pixels. However, this is ill-defined, (0,0) is the corner of an aperture in SCI coordinates, so negative locations will by definition be outside the aperture where the distortion model is not guaranteed to be physical.

Generally speaking there is no specific range that would be guaranteed to always be in the range of valid inputs for any aperture, so this test may need to be rethought (I’d be inclined to simply remove it).

Witchblade101 commented 2 years ago

Also causes the errors seen in generate_fgs.py:

Running aperture_transforms test for pre_delivery_siaf Traceback (most recent call last): File "/Users/dlong/pysiaf/generate/generate_fgs.py", line 261, in test_aperture.test_jwst_aperture_transforms([pre_delivery_siaf]) File "/Users/dlong/pysiaf/pysiaf/tests/test_aperture.py", line 154, in test_jwst_aperture_transforms assert error < threshold AssertionError

Witchblade101 commented 2 years ago

@tonysohn Everything is breaking because

assert np.allclose(fgs_aperture.sky_to_det(*fgs_aperture.det_to_sky(d1,d2)), (d1,d2)), "sky_to_det(det_to_sky) was not an identity"

in test_aperture.py always fails. And the other SIs are freaking out at all of the error messages every time they do a pull request. Is there any way we can fix that test, or is there any harm in commenting out that line if you expect it to always fail?

Or should we do as @drlaw1558 recommends and remove the entire test?

(I had PR that skipped this test in all of the generate scripts, but withdrew it because it still affects the CI.)

tonysohn commented 2 years ago

@Witchblade101 I suggest commenting out that line for now. We can take a look more later to see if increasing the threshold makes better sense.