"Analytic" non-redundant mask shape for NIRISS

[josePhoenix]

Includes commits from #160.

This PR adds an analytic optic for the NIRISS NRM, implemented much the same way as WebbPrimaryAperture. Instead of drawing all of the segments, it draws:

holey_segments = set(['C1-8', 'B2-9', 'B3-11', 'B4-13', 'C5-16', 'B6-17', 'C6-18'])

And contracts their outlines by 0.82 m / 1.34 m = 0.612x.

• [ ] Did I get the scale factor right?
• [ ] I added flip_x and flip_y arguments to the initializer for this optic. This seems wrong, but it's in keeping with the way add_pupil() was adding the MASK_NRM.fits.gz optic. I verified that the output is oriented correctly, so at least I'm not introducing a regression. What's the best way forward here?

[mperrin]

Scaling down a handful of segments is a clever way to do this, but we ought to be able to get the exact coordinates from the NIRISS team. In fact there's a version of those in one of the scripts in the webbpsf-data-source directory on central store. But I'm not 100% sure those are the final correct flight unit coordinates.

The flips are needed because you're defining this with respect to the entrance pupil OTE coordinates, but it's inserted in the SI pupil plane which is inverted with respect to the entrance pupil.

[mperrin]

Hmm, looks like the Travis build failed because this was branches during when I had master broken temporarily en route to getting the detector coords saved in FITS headers properly. Should get fixed if you merge or rebase onto current master, I expect.

[josePhoenix]

The flips are needed because you're defining this with respect to the entrance pupil OTE coordinates, but it's inserted in the SI pupil plane which is inverted with respect to the entrance pupil.

Right, I mentioned that because flip_x= and flip_y= appear to only be implemented for FITSOpticalElement, not generically for OpticalElement. I implemented them independently in the class I wrote, but it seems like there might be a better way using the CoordinateTransform stuff you added. Thoughts?

[mperrin]

Ah, OK. Good point. Yeah, you're right, that could easily be added in poppy to AnalyticOpticalElement.get_coordinates along with the shifts and rotations.

[mperrin]

And then change wave.coordinates() to self.get_coordinates(wave) in this

[mperrin]

One thing that's nice about the current FITS file is that it's a gray scale rather than binary mask, so in effect it has sub pixel resolution. Not sure how much difference that makes in practice, but some. So in that one detail this analytic version isn't as good as what's in the FITS file now. Right?

As you probably recall I've played around some with trying to get general-purpose anti-aliased geometry code for poppy, but never quite got that totally working well enough to use on a regular basis. I'd be curious to see a comparison case between a binary and gray scale version of this, to help inform how much we need to worry about this for NRM. But I'd put that on the "nice to have" wishlist rather than required for merging this.

[josePhoenix]

I had to keep the flip_x and flip_y implementations since I didn't see how to use self.get_coordinates(wave) would accomplish the required flips...

[mperrin]

Yeah. Looking back at my earlier suggestions, I eventually realized that we would have to add the flip_* options to the get_coordinates function in the parent class, and then it you could use it this way. Implementing the solution more generally rather than just in this class. But that's not the only way to do it of course

[mperrin]

I've decided we should not merge this for now, because it introduces significant changes in existing model results. The total area, and thus throughput, of the non-redundant mask changes by about 2.5%, and the hexagons all shift systematically further away from the telescope axis:

This results in local changes in the PSF of up to 5% of the peak pixel intensity.

I don't think it is justified to introduce that large a change in the model outputs without having any sense that it's a change for the better. In particular the existing NRM mask (the FITS file version) was created based on the specific mask coordinates for NIRISS, as provided by @anand0xff (in v2v3 coordinates to 4 decimal places). That's probably more accurate than this version which just symmetrically scales down primary mirror segments, by a scale factor we're not sure of.

[josePhoenix]

Fair enough!

[josePhoenix]

FYI, I believe this this was used to perform the OSIM pupil + OSIM WFE + NRM WebbPSF calculation in the CV3 data analysis, since the existing MASK_NRM file is a different sampling than the OSIM one.

[mperrin]

Now that I dig into this more, I'm reminded of my April 9th comment above. The right way to implement something like this would be to use the same coordinates (from webbpsf-data-source/NIRISS/sources/tfi_nrm_coords.txt), and use those in the calls to Path to define the geometry.

Yes - the fact that this was motivated by the OSIM comparison only (but wasn't requested for any other purpose) is the other reason why I don't think it's compelling to merge this. I.e. nobody will be sad if this isn't merged. So let's err on the side of less changes now in a rush. :-)

[anand0xff]

I agree with Marshall - we need methodical traceability in NRM definitions & descriptions, and concomitant careful steps towards a more realistic PSF.

OSIM has it's own pupil deformation, probably distinct from the flight-like NIRISS entrance pupil.

Anand

On Thu, Aug 10, 2017 at 6:13 PM, Marshall Perrin notifications@github.com wrote:

Now that I dig into this more, I'm reminded of my April 9th comment above. The right way to implement something like this would be to use the same coordinates (from webbpsf-data-source/NIRISS/sources/tfi_nrm_coords.txt), and use those in the calls to Path to define the geometry.

Yes - the fact that this was motivated by the OSIM comparison only (but wasn't requested for any other purpose) is the other reason why I don't think it's compelling to merge this. I.e. nobody will be sad if this isn't merged. So let's err on the side of less changes now in a rush. :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mperrin/webbpsf/pull/162#issuecomment-321687797, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOoCFbD5_o4FfvBimaGpIjaMhJYcHASks5sW4CMgaJpZM4M3z-D .

[anand0xff]

btw Johannes tweaked your mask creation code empirically to agree better with Alex' "LG" analyical approach... not sure where it's documented.

Anand

On Thu, Aug 10, 2017 at 10:14 PM, Anand Sivaramakrishnan < anand0xff@gmail.com> wrote:

I agree with Marshall - we need methodical traceability in NRM definitions & descriptions, and concomitant careful steps towards a more realistic PSF.

OSIM has it's own pupil deformation, probably distinct from the flight-like NIRISS entrance pupil.

Anand

On Thu, Aug 10, 2017 at 6:13 PM, Marshall Perrin <notifications@github.com

wrote:

Now that I dig into this more, I'm reminded of my April 9th comment above. The right way to implement something like this would be to use the same coordinates (from webbpsf-data-source/NIRISS/sou rces/tfi_nrm_coords.txt), and use those in the calls to Path to define the geometry.

Yes - the fact that this was motivated by the OSIM comparison only (but wasn't requested for any other purpose) is the other reason why I don't think it's compelling to merge this. I.e. nobody will be sad if this isn't merged. So let's err on the side of less changes now in a rush. :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mperrin/webbpsf/pull/162#issuecomment-321687797, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOoCFbD5_o4FfvBimaGpIjaMhJYcHASks5sW4CMgaJpZM4M3z-D .

[mperrin]

Closing out this old pull request, which we decided not to merge. See above comments from August '17.