Pupil mask property

[robelgeda]

Response to #203.

Settings:

• "AUTO" : Automatically select pupil
• "COLD_PUPIL": Masked pupil override
• "UNMASKED": Unmasked pupil override

[robelgeda]

@mperrin: This changes how the API sets the WFI pupil masks as requested by #203. The initial code sets the pupil by calling the toggle_pupil_mask function; now the mask is set using a property setter (e.g. WFI.pupil_mask = "COLD_PUPIL" ). I may have misunderstood the ticket though.

[mperrin]

The API change makes sense to me. (Let's ping @ariedel to double check this is what he wanted, but that part looks good to me). I'm just not sure about the need for 'MASKED' as a new synonym to 'COLD_PUPIL'. (recall PEP20: "There should be one-- and preferably only one --obvious way to do it.", etc)

Is there any standard terminology the project uses that we could be guided by?

[robelgeda]

Agreed, I think it makes sense to keep "AUTO" as the only option for the first setting. It up to @ariedel when one ("COLD_PUPIL" or "COLDPUPIL" or "COLD" or "MASKED") should set masks.

[ariedel]

This is what I wanted - a WFI.pupil_mask I can set with a string to activate the cold pupil mask.

[ariedel]

I would prefer COLD_PUPIL as the name.

[robelgeda]

@ariedel thanks! @mperrin I think its ready.

[mperrin]

Ok, thanks - I’ll wait for the Travis build then merge. Or should we add a unit test for any of this?

[robelgeda]

I can add tests. I am not sure why Travis is failing now though, It looks unrelated to this change.

[mperrin]

I did a bunch of other unrelated changes yesterday and the build failures may be due to that, although master is passing right now. Perhaps if you merged master into this it might work? I’ll take a closer look at the logs.

[mperrin]

Aha no, actually this is a real issue that the test is finding. Now that the API looks the same as the JWST instruments, it runs into a check for if pupil_mask is not None inside SpaceTelescopeInstrument._get_optical_system, which then wants to invoke a function to add optional coronagraph optics etc.

In the WFIRST case the pupil mask isn't added as another optical plane in the system, it's added by toggling the overall pupil mask. But for API compatibility it may be easiest to just add a stub placeholder function to the WFIRST class. Try adding the following.

   def _addAdditionalOptics(self,optsys, **kwargs):
        return optsys, False, None

That no-op in some sense stands for "don't add any coronagraph masks to WFI, even if pupil_mask has some value other than None". That will avoid invoking the superclass function that's raising a NotImplementedError right now in the test.

[mperrin]

Oh and while editing could you also please add a line:

self.pupil_mask_list = ['AUTO', 'COLD_PUPIL', 'UNMASKED']

That will continue to match the JWST instrument API and makes explicit the allowed values for that attribute.

[mperrin]

Working now! Thanks @robelgeda

[robelgeda]

@mperrin: sounds good, thanks for the hints!