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

Update or remove test_jwst_aperture_transforms #186

Open
mfixstsci opened this issue Jul 27, 2021 · 3 comments
Open

Update or remove test_jwst_aperture_transforms #186

mfixstsci opened this issue Jul 27, 2021 · 3 comments

Comments

@mfixstsci
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants