Updates to fresnel.py allowing users to create their own inwave object. #402
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #402 +/- ##
===========================================
+ Coverage 73.88% 74.10% +0.22%
===========================================
Files 17 17
Lines 5900 5885 -15
===========================================
+ Hits 4359 4361 +2
+ Misses 1541 1524 -17
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @kian1377,
I think we should consider making these changes in the parent class calc_psf instead. As much as possible we would like to keep the same consistent API for both the Fresnel and Fraunhofer classes. Furthermore, right now this adds a lot of code duplication, which makes maintenance harder in the future.
The inwave parameter is just as applicable to the Fraunhofer propagation class as well, yes? So I would like to request revising this to instead make the changes in poppy_core to add that parameter, which will provide that option consistently for both propagation methods as well as avoiding the need to duplicate 300+ lines of code.
Thanks!
|
Okay, I have been working on implementing it that way today and writing a couple tests to go along with those changes. |
inwave kwarg now works for standard and fresnel systems and tests have been written for validation.
|
Yeah, I just noticed that after I updated my repo and committed the
changes. I'll fix that real quick.
…On Fri, Mar 5, 2021 at 10:43 AM Ewan Douglas ***@***.***> wrote:
*External Email*
***@***.**** requested changes on this pull request.
it looks like this commit creates some duplicate files that need to be
moved back into their subdirectories
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#402 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMCLT2RZRNFUQJ6Q2TKRU2TTCEJ3LANCNFSM4YJJ7GEA>
.
|
Accidentally put some files in the wrong places when updating the inwave functionality causing duplicate files to be created, so just moved files around and placed them where they needed to be.
|
Hmm, I'm confused what's going on with the changed files display in this PR now. What I see there appears to be a copy of some of the changes from PR #384, and I don't see anything about the inwave parameter. Not sure what's going on. |
|
It looks like the tests that are failing are those for the compound systems, I will look into these later today and try and fix them. |
|
I am taking a look at the changes made and I don't see anything about PR #384, I never even had anything to do with that so I don't know whats going on there either. |
Updated the input_wavefront method in CompoundOpticalSystem to allow for the inwave kwarg to be used.
|
So all tests should be passing at this point because I ran the three tests related to the CompoundOpticalSystem's on my machine and they ran without error. Please let me know if there are more issues I need to resolve first. |
|
Hi @kian1377, it does look like there is some work work to resolve on this PR. In particular if you look at the GitHub display it shows there are conflicts, like this: Conflicting files That will have to be resolved. The best way to do that is for you to merge the current |
|
Hello, I will try and resolve the conflict within the next day. I am still getting used to github and don't fully understand pull requests and how to deal with different branches. I thought by updating the files in my repo, which I made the PR from, the PR would automatically update but it seems something went wrong this time. |
This update implements the inwave kwarg in the poppy_core.py and frensel.py files as well as adding the tests that were written.
|
Hello @mperrin , so I was trying the git merge upstream/develop command but I kept on running into issues with conflicts and had to jump through some hoops to update my local clone of poppy. In the end, I downloaded the zip code of spacetelescope/poppy develop branch and replaced all my files in my repo with those and committed the changes. Then, I made the edits to the poppy_core.py, frensel.py, and test_core.py files that implement the inwave kwarg and committed those changes as well. So now, there shouldn't be any conflicts and you should see the inwave updates. |
needed to include an import of matplotlib.pyplot so that the two additional tests for the inwave kwarg could run.
|
Hi @kian1377, there are still a few changes and some cleanup I'd like to make in this. I have the changes locally here. It might be easiest if I can just push these directly to your branch, which I have tried to do but ran into a permissions issue. Can you please double-check that the GitHub setting "allow edits from maintainers" is set for this pull request, please? See https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork |
|
Hello, the "allow edits from maintainers" option is already selected. I am still looking into options that may be preventing your changes from being pushed in my forked repo of poppy. |
|
I have unchecked an option for my forked repo saying "restrict editing to collaborators only." Maybe this was the option preventing the changes to be pushed directly to my branch. @mperrin when you get the chance, please try to push the changes again. If it still doesn't work, you could send me the files that you have changed via email at kianmilani@email.arizona.edu and I could push them to my repo. |
Slight edits to the way the inwave kwarg should be handled and the log messages to go with it.
|
@mperrin , I updated the PR with the files you sent me. I am not sure why git was giving us trouble with this but being new git and PRs, I'm pretty sure its something on my end that I just haven't figured out yet. |
There was a problem hiding this comment.
Looks good. Thanks again Kian both for implementing this and for working through the revisions with me.
(And I'm not sure why git is being a pain on this one - it's still showing in the diff duplicate copies of a few changes already in develop, for some reason - but it's not a roadblock. Git is not the most user-friendly piece of software in the world, alas.)
The changes made are such that a user can input their own custom wavefront data into a FresnelOpticalSystem() when using calc_psf().
The methods calc_psf() and propagate_mono() have been added directly into the FresnelOpticalSystem() class, so they are not just inherited and they contain an additional kwarg that is inwave. This kwarg is initialized to be None, but the user can define their own FrenselWavefront() and input it into calc_psf() using the inwave kwarg. This inwave object is then passed to propagate_mono(), which subsequently passes inwave to the input_wavefront() method in FresnelOpticalSystem(), which has also been edited to initialize a wavefront if the value of inwave is None, or to raise a value error if something other than a FresnelWavefront() object was supplied.
The changes made have been in an effort to create the Roman CGI modes with POPPY, however, the SPC modes and their data files required a unique sequence of MFTs and FFTs to apply the Focal Plane Masks (FPMs). So two separate optical systems were created, the first going up to the FPM and the second going through the rest of the optics of the CGI. This way, the FPM could be applied to the final wavefront of the first optical system, and then input into the second system to continue propagation. The results of this system have been tested and show agreement with the results of the PROPER model.
Future efforts will include implementing the technique used to apply the FPMs directly into POPPY such that two separate optical systems do not need to be created, but having the flexibility for a user to input their own wavefront data still seemed useful.
Please let me know if there issues with the PR or anything else I should change. Others who have contributed are @douglase and @Jashcraf.