JWInstrument._tel_coords returns incorrect V2/V3 angle

[JarronL]

In general, the JWInstrument class determines a set of detector pixel positions using set_position_from_aperture_name() which currently stores the raw detector pixel values as seen the det coordinate frame. However, when one attempts to convert those to telescope coordinates (tel) via the _tel_coords method (which in turn calls pix2angle in the DetectorGeometry class), this function instead assumes the input pixels are in the sci frame not the det frame:

tel_coords = np.asarray(self.aperture.sci_to_tel(xpix, ypix))

It's easy to where the confusion came from, because there are really two separate pixel coordinates (sci and det), so the pix2angle() function can be ambiguous if one isn't paying attention. I might recommend adding a keyword specifying the input coordinate frame along the lines of:

def pix2angle(self, xpix, ypix, input_frame="sci"):
 ...
 ...
 tel_coords = np.asarray(self.aperture.convert(xpix, ypix, input_frame, "tel"))
 ...
 ...

Edit:
Upon further investigation, it looks like WebbPSF expects detector_position to be in the science frame as opposed to the detector frame. In this case, the simplest solution is to modify set_position_from_aperture_name:

self.detector_position = (ap.XSciRef, ap.YSciRef)

I think my original point on adding a choice of input frame in pix2angle might still be useful, though.

[mperrin]

I think your edited/"upon further investigation" point is right. Yes, the intent of the detector_position parameter is it's in science frame, matching the data products observers will receive from DMS. I can easily believe I made a mistake when coding up the set_position_from_aperture_name function. So that's a very simple fix. @shanosborne can you take a look and see if you agree with this?

[shanosborne]

I looked this over and I agree. It looks like set_position_from_aperture_name() should set self.detector_position = (ap.XSciRef, ap.YSciRef), and then the docstrings of _tel_coords() and pix2angle() should be updated to state that they are converting from the science frame rather than the detector frame.

As for whether the pix2angle() method should be expanded to work for any conversion to the tel frame, it does say in the docstring (although right now not as accurately as it should) what the conversion is, but if we think there’s a concern of people not reading that then I suppose it doesn’t hurt.

[mperrin]

Thanks @shanosborne and @JarronL. Seems like we have a plan. Now we just need to agree which of us is going to turn that into a PR. Any volunteers? :-)

[shanosborne]

I can do it. @mperrin - do you have an opinion on if the pix2angle() method needs the extra argument or if a more explicit docstring is enough?

[mperrin]

I think the better docstring is a sufficient fix for now - we can add the new parameter later if necessary but right now it’s not clear to me it’s actually needed.

[mperrin]

Fixed by #281