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.

Sign up for GitHub

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

Jump to bottom

rationalize SpaceTelescopeInstrument.calcPSF with poppy.Instrument.calc_psf #132

Closed
mperrin opened this issue Sep 26, 2016 · 2 comments
Closed

rationalize SpaceTelescopeInstrument.calcPSF with poppy.Instrument.calc_psf #132

mperrin opened this issue Sep 26, 2016 · 2 comments
Labels
bug
Milestone
0.6.0

Comments

@mperrin
Copy link
Owner

mperrin commented Sep 26, 2016

This got overlooked in the API updating. Whoops...

See mperrin/poppy#180 (comment) :

just realized to my horror that when we updated the API for calcPSF->calc_psf we never updated SpaceTelescopeInstrument. So I've been doing all my testing lately calling calc_psf which was just calling the parent Instrument.calc_psf. It turns out that has been working fine - so I'm now confused and can't remember why we overrode that function in SpaceTelescopeInstrument.
@mperrin mperrin added the bug label Sep 26, 2016
@mperrin mperrin added this to the 0.5.1 milestone Sep 26, 2016
@mperrin
Copy link
Owner Author

mperrin commented Sep 26, 2016

After fixing a trivial typo in a warning string in poppy.Instrument, it seems that most webbpsf tests are passing with calls to Instrument.calc_psf instead of SpaceTelescopeInstrument.calcPSF (hereafter STI.calcPSF for brevity).

What's different between those two functions?

  • STI.calcPSF has support for a couple deprecated input parameters which we can just do away with now.
  • STI.calcPSF allows specification of rectangular rather than square fields of view. We should move this bit of useful code to Instrument.calc_psf.
  • if oversampling is not explicitly set, STI.calcPSF reads a default from the package configuration settings, while Instrument.calc_psf has it hard-coded to 4.
  • if display is True, STI.calcPSF has slightly different plot annotation text than Instrument.calc_psf does.

None of those seem sufficient to justify separate functions at this point. I think this is pretty much just vestigial code and we can & should simplify it down to just one function.

mperrin added a commit to mperrin/poppy that referenced this issue Sep 26, 2016
@mperrin
merge some useful bits from webbpsf.SpaceTelescopeInstrument.calcPSF;…
ee4db94 
… see mperrin/webbpsf#132
@mperrin
Copy link
Owner Author

mperrin commented Sep 26, 2016

Fixed (in the si_wfe branch for now, will merge later to master.

@mperrin mperrin mentioned this issue Oct 11, 2016
Closed
josePhoenix pushed a commit to josePhoenix/webbpsf that referenced this issue Oct 28, 2016
@mperrin
Overlay changes from 0.6.0rc1 for quick patch release
90c8136 
 - better document PSF normalization options. Addresses mperrin#112
 - update travis config, consistent with poppy#187
 - display tweak for primary V2V3 annotation
 - remove redundant calcPSF in favor of just using the superclass one; fixes mperrin#132
 - update measure_strehl to turn off SI WFE for perfect PSF calcs
 - Enforce Python 3.0+ compliance on code with __future__ imports
 - Use six.string_types for Python 3.x compliance
 - Add versions + jwxml to install_requires in setup.py
 - Drop matplotlib requirement to >=1.5.0 in setup.py
josePhoenix pushed a commit to josePhoenix/webbpsf that referenced this issue Oct 28, 2016
@mperrin
Overlay changes from 0.6.0rc1 for quick patch release
c3f02c4 
 - a few missed version number->0.5.0 edits in install docs  [ci skip]
 - Update install instructions for Ureka->Astroconda change [skip ci]
 - Formatting fix installation.rst [skip ci]
 - Further revisions to install docs for AstroConda [skip ci]
 - Clarify release instructions for data packages
 - Fix ConfigParser import in setup.py
 - Back to development: 0.5.1
 - better document PSF normalization options. Addresses mperrin#112
 - update travis config, consistent with poppy#187
 - display tweak for primary V2V3 annotation
 - remove redundant calcPSF in favor of just using the superclass one; fixes mperrin#132
 - update measure_strehl to turn off SI WFE for perfect PSF calcs
 - Enforce Python 3.0+ compliance on code with __future__ imports
 - Use six.string_types for Python 3.x compliance
 - Add versions + jwxml to install_requires in setup.py
 - Drop matplotlib requirement to >=1.5.0 in setup.py
josePhoenix pushed a commit to josePhoenix/webbpsf that referenced this issue Oct 29, 2016
@mperrin
Overlay changes from 0.6.0rc1 for quick patch release
e28cdaa 
 - a few missed version number->0.5.0 edits in install docs  [ci skip]
 - Update install instructions for Ureka->Astroconda change [skip ci]
 - Formatting fix installation.rst [skip ci]
 - Further revisions to install docs for AstroConda [skip ci]
 - Clarify release instructions for data packages
 - Fix ConfigParser import in setup.py
 - Back to development: 0.5.1
 - better document PSF normalization options. Addresses mperrin#112
 - update travis config, consistent with poppy#187
 - display tweak for primary V2V3 annotation
 - remove redundant calcPSF in favor of just using the superclass one; fixes mperrin#132
 - update measure_strehl to turn off SI WFE for perfect PSF calcs
 - Enforce Python 3.0+ compliance on code with __future__ imports
 - Use six.string_types for Python 3.x compliance
 - Add versions + jwxml to install_requires in setup.py
 - Drop matplotlib requirement to >=1.5.0 in setup.py
@mperrin mperrin mentioned this issue Dec 5, 2016
Closed
@mperrin mperrin mentioned this issue Aug 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
0.6.0
Development

No branches or pull requests
1 participant
@mperrin