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

Wfe patch #311

Merged
merged 3 commits into from Apr 23, 2019
Merged

Wfe patch #311

merged 3 commits into from Apr 23, 2019

Conversation

douglase
Copy link
Collaborator

H/T to @jlumbres for the advance warning, here's a quick, if inelegant fix of #310.

@coveralls
Copy link

coveralls commented Apr 11, 2019

Pull Request Test Coverage Report for Build 101

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 72.561%

Totals Coverage Status
Change from base Build 99: 0.005%
Covered Lines: 3845
Relevant Lines: 5299

💛 - Coveralls

poppy/wfe.py Outdated
@@ -33,7 +35,7 @@ def _accept_wavefront_or_meters(f):

@wraps(f)
def wrapper(*args, **kwargs):
if not isinstance(args[1], Wavefront):
if not ( isinstance(args[1], FresnelWavefront) or isinstance(args[1], Wavefront)):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right fix here is to use the superclass: if not isinstance(args[1], BaseWavefront) (along with adding an import of BaseWavefront above.)

@douglase, I can go ahead and make that change myself and we can discard this PR, or you can change the PR for it. Either way is fine with me so just let me know how you'd like to proceed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglase did you get a chance to see my comment last week on this? Let me know your preference and I'll work on this today or tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see it earlier, change made.

@douglase
Copy link
Collaborator Author

I'm a little concerned that the newly initialized wavefront might cause a problem if folks want a different wavefront class than the one initialized.

@mperrin
Copy link
Collaborator

mperrin commented Apr 23, 2019

good question. In the normal use case, a Wavefront or FresnelWavefront is handed in explicitly. This fallback back only gets used in the case that someone says e.g. optic.get_opd(2e-6). So in that case it's hard to say "folks want a different case than the one initialized" if they're just providing a wavelength.

I can't remember which use case(s) motivate still supporting that back compatible path that just takes a wavelength. Maybe the right fix here is deprecating that entirely? But clearly it gets used somewhere in the execution path that leads to #310.

@douglase
Copy link
Collaborator Author

#310 is passing a wavefront, but since the type was not correct it was trying to create a wavefront by passing a wavefront as the wavelength, which returned an error. I expect it is useful to pass a wavelength to an WFE object for illustrative or diagnostic purposes and it might be worth leaving as is for that case, since it's unlikely as user who hasn't initialized a wavefront will care what type the returned wavefront is.

@mperrin
Copy link
Collaborator

mperrin commented Apr 23, 2019

OK, I'm going to go ahead and merge in this PR now. I think I will open up a separate issue that we should consider removing/deprecating this decorator function - it's not consistent with the rest of the API to be able to pass wavelengths and have it automatically turn into a wavefront, and I can't remember what motivated this to begin with.

@mperrin mperrin merged commit 165f590 into spacetelescope:master Apr 23, 2019
@mperrin mperrin added this to the 0.9.0 milestone Aug 8, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants