Adding phase retrieval

[tobias-liaudat]

Summary

Closes #67. Closes #91. Closes #132.

Issue #66 requires the physical layer from #123.

This PR adds the phase retrieval capabilities including a new projection algorithm that takes into account the obscurations.

Added unit tests for the new projection function, tf_decompose_obscured_opd_basis, and the original PI_zernikes function.

Validation Test Reports

• The code was validated against the v1 version of WaveDiff, with the release v1.3.0 https://github.com/tobias-liaudat/wf-psf/tree/v1.3.0 using the same random seeds and getting the exact same metrics. The release v1.3.0 resolves #91. Adding both metric log files as comparison:

  • metrics_wfv1_seed_v0.log
  • metrics_wfv2_seed_v0.log

• All unit tests passing (without requiring gpu).

• All code formatted with black.

Reviewer Checklist

Reviewers should tick the following boxes before approving and merging the PR.

• [x] The PR targets the develop branch
• [x] The PR is assigned to the developer
• [x] The PR has appropriate labels
• [x] The PR is included in appropriate projects and/or milestones
• [x] The PR includes a clear description of the proposed changes
• [x] If the PR addresses an open issue the description includes "closes #"
• [ ] The code and documentation style match the current standards
• [ ] Documentation has been added/updated consistently with the code
• [x] All CI tests are passing
• [ ] API docs have been built and checked at least once (if relevant)
• [x] All changed files have been checked and comments provided to the developer
• [ ] All of the reviewer's comments have been satisfactorily addressed by the developer
• [ ] All Validation tests for the reported error are passing
• [ ] I have reviewed the CI log to verify if any files require formatting with Black. If any files are found to be improperly formatted, I have provided comments indicating the need for formatting.

[tobias-liaudat]

Thanks for the detailed feedback @jeipollack :)

I went through the comments and pushed several changed to the code addressing most of the comments.

Concerning the comment:

• Refactor long class method project_DD_features into smaller, more modular functions to improve readability and maintainability.

I don't think it is a good idea. Encapsulating the steps of that function in my opinion will not improve readibility and maintainability, but the opposite. The steps should not be modular functions as they are specific for that function, and breaking down into smaller functions will only make it harder to understand and maintain. It will end up as ravioli code, in my opinion.

Concerning the unit tests: I removed one unnecessary dependecy. The proposed tests are testing what each of the functions are doing, projection WFE into a zernikes, for an unobscured and an obscured pupil. If these tests are passed we should be confident that the scientific purpose of those two functions is accomplished, which is the least minimum we need. There are some dependencies, e.g. zernike_generator or SimPSFToolkit.generate_pupil_obscurations, that I don't see how to get rid of (without copy-pasting their underlying code). I also don't see how to break it into smaller tests (see unobscured_zernike_projection for example). It seems to me that that will only help to test some edge cases.

Given the context, time constraints and manpower we have I don't think it is a priority to invest more time in these tests and I don't plan to do it for the moment.

[jeipollack]

@tobias-liaudat That's why they are called helper functions. Breaking the functions down into independent parts allows them to be tested with actual unit tests and possibly reused.

It seems that there is a lot of code duplication and utility functions dispersed all through out the code base, which is what I want to clean up and avoid.

As for your unit tests, I wasn't asking you to redo them. I was stating to you that they are not unit tests.

[tobias-liaudat]

@jeipollack regardless of their name, I don't think they're helping in this case. I don't see the point in encapsulating specific tensor multiplications, as suggested. It will make it very hard to understand later, and there is still a lot of scientific development and features needed for wavediff.

We can think of refactoring it at a later point, if we believe it will be reused later.

[jeipollack]

Note I emphasised the importance more in terms of testability. Functions should be small and limited in scope. This is one of the standard good practices in software development and maintenance. You admitted that you're not paying attention to the line coverage. Well, the SGS software quality control evaluates this, and I was hired to do the integration of this framework into the SGS pipeline. There needs to be a compromise between quality and quick delivery. If you will not implement at least some of the requests in terms of refactoring, I will do it myself or ask Nada to do it when I or she complete the other tasks.

I politely request that for future development you please consider writing your functions in smaller more focused units.